Le 08/05/2017 à 22:09, Samuel Thibault a écrit :
> Hello,
>
> Laurent Vivier, on sam. 06 mai 2017 15:19:44 +0200, wrote:
>>> Laurent Vivier, on sam. 06 mai 2017 00:48:33 +0200, wrote:
>>>> @@ -617,6 +622,10 @@ void slirp_pollfds_poll(GArray *pollfds, int
>>>> select_error)
>>>> * Check sockets for reading
>>>> */
>>>> else if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) {
>>>> + if (so->so_state & SS_ISFCONNECTING) {
>>>> + socks5_recv(so->s, &so->so_proxy_state);
>>>> + continue;
>>>> + }
>>>
>>> Again, I don't see how this can work with both socks5 case and
>>> non-socks5 case. Don't we need to somehow check for the type of socket
>>> before calling socks5_recv?
>>
>> The check is done previously by:
>>
>> @@ -442,6 +443,10 @@ void slirp_pollfds_fill(GArray *pollfds, uint32_t
>> *timeout)
>> .fd = so->s,
>> .events = G_IO_OUT | G_IO_ERR,
>> };
>> + if (so->so_proxy_state &&
>> + so->so_proxy_state != SOCKS5_STATE_ERROR) {
>> + pfd.events |= G_IO_IN;
>> + }
>>
>> We can enter in socks5_recv() only if so->so_proxy_state is in a valid
>> state because G_IO_IN triggers that.
>
> I don't understand: the socks5_recv gets called not only when pfd.events
> contains G_IO_IN, but also when it contains G_IO_HUP or G_IO_ERR. Also,
> so_proxy_state being non-nul is not the only way to have G_IO_IN set in
> pfd.events, so_state & SS_FACCEPTCONN and CONN_CANFRCV(so) also trigger
> that.
This is filtered by (so_state & SS_ISFCONNECTING). The only case when we
enter in the receiving part with SS_ISFCONNECTING is when socks5 part
has been enabled.
>>>> @@ -645,11 +654,19 @@ void slirp_pollfds_poll(GArray *pollfds, int
>>>> select_error)
>>>> /*
>>>> * Check for non-blocking, still-connecting sockets
>>>> */
>>>> - if (so->so_state & SS_ISFCONNECTING) {
>>>> - /* Connected */
>>>> - so->so_state &= ~SS_ISFCONNECTING;
>>>>
>>>> - ret = send(so->s, (const void *) &ret, 0, 0);
>>>> + if (so->so_state & SS_ISFCONNECTING) {
>>>> + ret = socks5_send(so->s, slirp->proxy_user,
>>>
>>> Ditto.
>>
>> if so_proxy_state is 0 the function does nothing
>
> Well, that's quite weak reason to me, and will confuse readers, because
> they'll think that the code is for the socks5 case only.
OK, I'm going to add a "if (so->so_proxy_state && so->so_proxy_state !=
SOCKS5_STATE_ERROR)" here.
>
>>>> +++ b/slirp/socks5.c
>>>> @@ -0,0 +1,371 @@
>>>
>>> In v2 of the patch, this was said to have "some parts from nmap/ncat
>>> GPLv2". Is that really not true any more? If any part of the file is
>>> not original, it *has* to wear proper copyright notices, otherwise it's
>>> copyright infrigement.
>>
>> No, I've re-written entirely this part from RFC.
>
> Ok.
>
>> for ncat.h
>
> Which code comes from ncat.h?
I haven't been clear: none. I've entirely re-written this part.
> Again, we can't copy/paste code like that, that is copyright
> infrigement.
>
>> license is 281 lines long (with clarifications and
>> extensions) for 60 lines copied...
>
> That is really no reason. Copyright thingies are considered as soon
> as dozen-ish lines of non-trivial code. The size of the terms of the
> licence itself really doesn't matter. If we don't respect others'
> copyright, we can't expect others (e.g. companies) to respect our GPL
> code.
I respect others copyright. There is no more nmap/ncat code in this patch.
Thanks,
Laurent
