Re: [Qemu-devel] [PATCH v4 1/1] slirp: add SOCKS5 support

[Samuel Thibault]

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