Patchwork tests: fix toctou race in tinyproxy.py (issue3795)

login
register
mail settings
Submitter Mads Kiilerich
Date Jan. 31, 2013, 7:01 p.m.
Message ID <9f4ee36491d826a07141.1359658900@mk-desktop>
Download mbox | patch
Permalink /patch/775/
State Accepted
Commit ca430fb6a668e26e929793b15b8cb5ce211272df
Headers show

Comments

Mads Kiilerich - Jan. 31, 2013, 7:01 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1359655993 -3600
# Branch stable
# Node ID 9f4ee36491d826a0714164721e631f026d63b324
# Parent  2a1fac3650a5b4d650198604c82ab59969500374
tests: fix toctou race in tinyproxy.py (issue3795)

test-http-proxy.t sometimes failed with:
    File ".../tests/tinyproxy.py", line 110, in _read_write
      data = i.recv(8192)
  error: (104, 'Connection reset by peer')

This might have started showing up with a9fd11ffa13f ... but it has apparently
also been seen before. I don't see anything in a9fd11ffa13f that can explain
it. It seems to be a race in test, in the tinyproxy helper:

Tinyproxy found an incoming socket using select(). It would break the loop if
an error had been detected on the socket, but there was no error and it tried
to recv() from the socket. That failed - apparently because it had been reset
after select().

Errors in the recv() will now be caught and will break the loop like errors
detected by select() would.

(send() could also fail in a similar way ... but using the same solution there
and losing data we have read doesn't feel right.)
Matt Mackall - Jan. 31, 2013, 8:06 p.m.
On Thu, 2013-01-31 at 20:01 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1359655993 -3600
> # Branch stable
> # Node ID 9f4ee36491d826a0714164721e631f026d63b324
> # Parent  2a1fac3650a5b4d650198604c82ab59969500374
> tests: fix toctou race in tinyproxy.py (issue3795)

Looks good.
Augie Fackler - Jan. 31, 2013, 9:12 p.m.
On Jan 31, 2013, at 3:06 PM, Matt Mackall <mpm@selenic.com> wrote:

> On Thu, 2013-01-31 at 20:01 +0100, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1359655993 -3600
>> # Branch stable
>> # Node ID 9f4ee36491d826a0714164721e631f026d63b324
>> # Parent  2a1fac3650a5b4d650198604c82ab59969500374
>> tests: fix toctou race in tinyproxy.py (issue3795)
> 
> Looks good.

FWIW, this freaks me out a little bit - I don't understand why there'd be a econnreset here. I may just not have spent enough time squinting at the code.

> 
> -- 
> Mathematics is the supreme nostalgia of our time.
> 
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Mads Kiilerich - Jan. 31, 2013, 9:31 p.m.
On 01/31/2013 10:12 PM, Augie Fackler wrote:
>
> On Jan 31, 2013, at 3:06 PM, Matt Mackall <mpm@selenic.com 
> <mailto:mpm@selenic.com>> wrote:
>
>> On Thu, 2013-01-31 at 20:01 +0100, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com <mailto:madski@unity3d.com>>
>>> # Date 1359655993 -3600
>>> # Branch stable
>>> # Node ID 9f4ee36491d826a0714164721e631f026d63b324
>>> # Parent  2a1fac3650a5b4d650198604c82ab59969500374
>>> tests: fix toctou race in tinyproxy.py (issue3795)
>>
>> Looks good.
>
> FWIW, this freaks me out a little bit - I don't understand why there'd 
> be a econnreset here. I may just not have spent enough time squinting 
> at the code.

Me neither.

It could in principle be "poor man's don't continue" ... but I don't see 
that happen in that test.

But the problem we see is that _if_ it (or some other error) happens, 
then it triggers a bug in tinyproxy. So I'm not sure if it is good or 
bad to fix that bug.

/Mads
Matt Mackall - Jan. 31, 2013, 11:09 p.m.
On Thu, 2013-01-31 at 16:12 -0500, Augie Fackler wrote:
> 
> On Jan 31, 2013, at 3:06 PM, Matt Mackall <mpm@selenic.com> wrote:
> 
> > On Thu, 2013-01-31 at 20:01 +0100, Mads Kiilerich wrote:
> > > # HG changeset patch
> > > # User Mads Kiilerich <madski@unity3d.com>
> > > # Date 1359655993 -3600
> > > # Branch stable
> > > # Node ID 9f4ee36491d826a0714164721e631f026d63b324
> > > # Parent  2a1fac3650a5b4d650198604c82ab59969500374
> > > tests: fix toctou race in tinyproxy.py (issue3795)
> > 
> > Looks good.
> 
> 
> FWIW, this freaks me out a little bit - I don't understand why there'd
> be a econnreset here. I may just not have spent enough time squinting
> at the code.

That occurs when we try to write to a socket the remote side has closed.
So that's maybe a real bug in either our client (writing too much) or
our server (hanging up early).

But the proximal bug is actually in the select loop in tinyproxy. It
depends on exceptional conditions to exit the loop, but it's perfectly
valid for ECONNRESET to show up in the read vector. The vagarities of
scheduling and flow control may make this happen differently under load.
        
        From Richard Stevens (rstevens@noao.edu):
        
        [...] What this also means is that if you were using select
        instead of write, the select would have indicated the socket as
        being readable, since the RST is there for you to read (read
        will return an error with errno set to ECONNRESET).

So I think this fix is correct, at least locally.
Matt Mackall - Feb. 1, 2013, 8:39 p.m.
On Thu, 2013-01-31 at 20:01 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1359655993 -3600
> # Branch stable
> # Node ID 9f4ee36491d826a0714164721e631f026d63b324
> # Parent  2a1fac3650a5b4d650198604c82ab59969500374
> tests: fix toctou race in tinyproxy.py (issue3795)

Queued for stable, thanks.

Patch

diff --git a/tests/tinyproxy.py b/tests/tinyproxy.py
--- a/tests/tinyproxy.py
+++ b/tests/tinyproxy.py
@@ -107,7 +107,10 @@ 
                         out = self.connection
                     else:
                         out = soc
-                    data = i.recv(8192)
+                    try:
+                        data = i.recv(8192)
+                    except socket.error:
+                        break
                     if data:
                         out.send(data)
                         count = 0