Hello,
Laurent Vivier, on mar. 09 mai 2017 09:21:09 +0200, wrote:
> Le 08/05/2017 à 22:09, Samuel Thibault a écrit :
> > Laurent Vivier, on sam. 06 mai 2017 15:19:44 +0200, wrote:
> >>> Laurent Vivier, on sam. 06 mai 2017 00:48:33 +0200, wrote:
> >> 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.
The code snippet above is guarded by (so_state & SS_ISFCONNECTING),
but that won't prevent socks5_recv from being called here in
slirp_pollfds_poll in the non-socks5 case:
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;
+ }
/*
e.g. when (so->so_state & SS_FACCEPTCONN) or when CONN_CANFRCV(so) in
slirp_pollfds_fill, which both set G_IO_IN too.
> >>>> @@ -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.
Thanks :)
> >>>> +++ 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.
Ah, ok.
Samuel
