[tor-bugs] #14135 [meek]: Incorrect SocksListener temporary error check

[Tor Bug Tracker & Wiki]

#14135: Incorrect SocksListener temporary error check
------------------------+-----------------
     Reporter:  adam-p  |      Owner:  dcf
         Type:  defect  |     Status:  new
     Priority:  minor   |  Milestone:
    Component:  meek    |    Version:
   Resolution:          |   Keywords:
Actual Points:          |  Parent ID:
       Points:          |
------------------------+-----------------

Comment (by dcf):

 Thanks for looking at this.

 Currently, the code works like this:
 {{{
 non-net.Error:                try again
     net.Error, Temporary:     try again
     net.Error, non-Temporary: quit
 }}}
 The patch changes it to be this:
 {{{
 non-net.Error:                quit
     net.Error, Temporary:     try again
     net.Error, non-Temporary: quit
 }}}
 The problem is that at least once in the past, we wanted to try again on a
 non-net.Error. See this commit message:
 https://gitweb.torproject.org/pluggable-
 transports/goptlib.git/commit/?id=3030f080eecf72b0e896236fca5fabd245c00bdb
 Before that commit (and with the patch in this ticket), you can kill a
 client process by sending something unexpected in the SOCKS request, for
 example:
 {{{
 socat -v - SOCKS4A:127.0.0.1:1.1.1.1:2222,socksport=<meek-client
 port>,socksuser=bogus
 }}}
 The error you get is this, which comes from lower level goptlib code, is
 not a net.Error:
 {{{
 2015/01/09 12:40:59 no equals sign in "bogus"
 }}}

 We can agree that we shouldn't kill the process in this case. But there
 are a few ways we could do it. One way is to continue the way that it
 works now, and document that errors that are not net.Errors should be
 treated as Temporary. Another way is for AcceptSocks to intercept the
 error returned by [https://gitweb.torproject.org/pluggable-
 transports/goptlib.git/tree/socks.go?id=99ea2c51f294fbd4cb016d96acde1b4288fc63e7#n158
 readSocks4aConnect] and, if it is not an net.Error, transform it into a
 Temporary net.Error. Failure to read/parse the connect message is arguably
 a network error.

 I think I would prefer a patch to do the second thing. Basically
 everywhere in readSocks4aConnect that returns fmt.Errorf, return a
 Temporary net.Error instead. The errors from underlying network calls
 should be returned without alteration. We should make sure that errors in
 other functions are consistent, and also see what the standard library
 does in similar cases (i.e., how does it construct Temporary errors).

 The upstream for goptlib is https://gitweb.torproject.org/pluggable-
 transports/goptlib.git/ BTW.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/14135#comment:1>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online