Re: Polipo request times out; and polipo crashes

David Röthlisberger <[email protected]> wrote:

> On 9 Nov 2012, at 01:05, Urs Holzer wrote:
> > I can confirm both issues on Linux x86_64 (by running Curl's test case 9 
> > against polipo from master branch).

I reproduced this (and other crashes) on FreeBSD amd64.

The attached patches 1 to 3 seem to prevent the crashes
for me, patch 4 is an unrelated bonus.

Curl's tests seem to show a couple of additional issues,
but I haven't analyzed them yet:

==8718== Syscall param write(buf) points to unaddressable byte(s)
==8718==    at 0x136F43A: __sys_write (write.S:3)
==8718==    by 0x136EEEF: __swrite (stdio.c:71)
==8718==    by 0x136EE3C: _swrite (stdio.c:133)
==8718==    by 0x136DCC0: __sfvwrite (fvwrite.c:89)
==8718==    by 0x136DBD9: fwrite (fwrite.c:95)
==8718==    by 0x40E6D7: really_do_log_n (log.c:476)
==8718==    by 0x425A33: httpWriteRequest (server.c:1620)
==8718==    by 0x42359C: httpServerTrigger (server.c:846)
==8718==    by 0x42268A: httpMakeServerRequest (server.c:475)
==8718==    by 0x425565: httpServerRequest (server.c:1509)
==8718==    by 0x41ECFF: httpClientNoticeRequest (client.c:1251)
==8718==    by 0x41DF4E: httpClientRequestContinue (client.c:946)
==8718==  Address 0xca000 is not stack'd, malloc'd or (recently) free'd
[...]
==8718== 38 bytes in 2 blocks are definitely lost in loss record 101 of 297
==8718==    at 0x104B33B: malloc (vg_replace_malloc.c:266)
==8718==    by 0x402A1C: strdup_n (util.c:30)
==8718==    by 0x42DD1F: httpParseHeaders (http_parse.c:1037)
==8718==    by 0x4268BB: httpServerHandlerHeaders (server.c:1931)
==8718==    by 0x426364: httpServerReplyHandler (server.c:1818)
==8718==    by 0x40724F: do_scheduled_stream (io.c:366)
==8718==    by 0x405DD4: eventLoop (event.c:718)
==8718==    by 0x414F39: main (main.c:165)
==8718== 
==8718== 658 bytes in 14 blocks are definitely lost in loss record 290 of 297
==8718==    at 0x104B33B: malloc (vg_replace_malloc.c:266)
==8718==    by 0x405F87: conditionWait (event.c:760)
==8718==    by 0x423FA5: httpServerDoSide (server.c:1040)
==8718==    by 0x42436B: httpServerSideHandlerCommon (server.c:1132)
==8718==    by 0x4243D7: httpServerSideHandler2 (server.c:1149)
==8718==    by 0x40724F: do_scheduled_stream (io.c:366)
==8718==    by 0x406952: schedule_stream (io.c:205)
==8718==    by 0x406426: do_stream_2 (io.c:81)
==8718==    by 0x423DB4: httpServerDoSide (server.c:1002)
==8718==    by 0x423C0C: httpServerSideRequest (server.c:975)
==8718==    by 0x423444: httpServerTrigger (server.c:800)
==8718==    by 0x422DFB: httpServerConnectionHandlerCommon (server.c:647)
==8718== 
==8718== 2,520 bytes in 21 blocks are definitely lost in loss record 293 of 297
==8718==    at 0x104B33B: malloc (vg_replace_malloc.c:266)
==8718==    by 0x41A571: httpMakeConnection (http.c:557)
==8718==    by 0x41BEC9: httpAccept (client.c:95)
==8718==    by 0x407F5F: do_scheduled_accept (io.c:677)
==8718==    by 0x405DD4: eventLoop (event.c:718)
==8718==    by 0x414F39: main (main.c:165)
==8718== 
==8718== 3,084 (2,400 direct, 684 indirect) bytes in 12 blocks are definitely lost in loss record 295 of 297
==8718==    at 0x104B33B: malloc (vg_replace_malloc.c:266)
==8718==    by 0x40B2A5: makeObject (object.c:193)
==8718==    by 0x41DE13: httpClientRequestContinue (client.c:909)
==8718==    by 0x434D8F: urlForbidden (forbidden.c:482)
==8718==    by 0x41DA86: httpClientRequest (client.c:824)
==8718==    by 0x41D3A7: httpClientHandlerHeaders (client.c:666)
==8718==    by 0x41C9BB: httpClientHandler (client.c:392)
==8718==    by 0x40724F: do_scheduled_stream (io.c:366)
==8718==    by 0x405DD4: eventLoop (event.c:718)
==8718==    by 0x414F39: main (main.c:165)

> I have a meta-comment regarding your use of curl's test suite:
> 
> It would be great if you can automate the running of curl's test suite
> (or a subset of it) through polipo so that it can be run with "make
> check" from the polipo source directory. Perhaps use an environment
> variable to specify the location of the curl source files (or an
> environment variable to specify a tag or URL of the curl source code to
> checkout into a temporary directory).

I've been using curl's test framework for a couple of years now
to test Privoxy and intend to upstream my modifications in the
near future after some more polishing.

Once that's done, using the tests with Polipo should be more
convenient as well.

> An easy way to share that work would be to fork polipo on github and
> commit to a branch on your fork, then post the link here. I'd help out
> when I can (for example with diagnosing failing tests; I expect a lot of
> them will fail, but not always due to a fault of polipo's).

You expect correctly:
TESTDONE: 35 tests out of 249 reported OK: 14%

Only executing the HTTP tests that are already known to work with Privoxy:
TESTDONE: 34 tests out of 211 reported OK: 16%

To be clear, this is not a fair comparison because I already
patched runtests.pl to prevent false positives when using Privoxy
but haven't patched it to deal with false positives when using
Polipo (yet).

Fabian

------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

_______________________________________________
Polipo-users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/polipo-users

From d566738e60087307168756dd53fba703469a6f56 Mon Sep 17 00:00:00 2001
From: Fabian Keil <[email protected]>
Date: Wed, 14 Nov 2012 21:40:31 +0100
Subject: [PATCH 1/4] Fix a segmentation fault that was reproducible with
 curl's test #9
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The issue was previously reported by Ming Fu and David Röthlisberger.
---
 server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/server.c b/server.c
index 408b910..6d0e972 100644
--- a/server.c
+++ b/server.c
@@ -1153,7 +1153,8 @@ httpServerContinueConditionHandler(int status, ConditionHandlerPtr chandler)
 {
     HTTPConnectionPtr connection = *(HTTPConnectionPtr*)chandler->data;
 
-    if(connection->request->flags & REQUEST_WAIT_CONTINUE)
+    if(!connection->request ||
+       connection->request->flags & REQUEST_WAIT_CONTINUE)
         return 0;
     httpServerDelayedDoSide(connection);
     return 1;
-- 
1.8.0

From e7b6ba4c326243ca1eeeb5251646c6a2546b7c86 Mon Sep 17 00:00:00 2001
From: Fabian Keil <[email protected]>
Date: Wed, 14 Nov 2012 22:07:11 +0100
Subject: [PATCH 2/4] Stop asserting that released objects have no condition
 handlers

The assertions were reproducible triggered by curl's test 9.
---
 object.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/object.c b/object.c
index ee5350f..4ab8c9e 100644
--- a/object.c
+++ b/object.c
@@ -288,8 +288,7 @@ releaseObject(ObjectPtr object)
            (unsigned long)object, object->refcount);
     object->refcount--;
     if(object->refcount == 0) {
-        assert(!object->condition.handlers && 
-               !(object->flags & OBJECT_INPROGRESS));
+        assert(!(object->flags & OBJECT_INPROGRESS));
         if(!(object->flags & OBJECT_PUBLIC))
             destroyObject(object);
     }
@@ -642,8 +641,7 @@ destroyObject(ObjectPtr object)
     int i;
 
     assert(object->refcount == 0 && !object->requestor);
-    assert(!object->condition.handlers && 
-           (object->flags & OBJECT_INPROGRESS) == 0);
+    assert((object->flags & OBJECT_INPROGRESS) == 0);
 
     if(object->disk_entry)
         destroyDiskEntry(object, 0);
-- 
1.8.0

From 9b402037ad46dcf4d4c6886dfb5208c12f324509 Mon Sep 17 00:00:00 2001
From: Fabian Keil <[email protected]>
Date: Wed, 14 Nov 2012 22:30:46 +0100
Subject: [PATCH 3/4] Do not call httpServerDoSide() from
 httpClientDelayedDoSideHandler() with a NULL request

Prevents a segfault reproducible with curl's test 1129.
---
 server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/server.c b/server.c
index 6d0e972..73bee95 100644
--- a/server.c
+++ b/server.c
@@ -1060,7 +1060,8 @@ static int
 httpClientDelayedDoSideHandler(TimeEventHandlerPtr event)
 {
     HTTPConnectionPtr connection = *(HTTPConnectionPtr*)event->data;
-    httpServerDoSide(connection);
+    if (connection->request)
+        httpServerDoSide(connection);
     return 1;
 }
 
-- 
1.8.0

From 5fecfadccd54e6ee83ff68f1531e78b6a4d2e735 Mon Sep 17 00:00:00 2001
From: Fabian Keil <[email protected]>
Date: Wed, 14 Nov 2012 23:30:52 +0100
Subject: [PATCH 4/4] Fix a couple of memory leaks in httpParseHeaders()

---
 http_parse.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/http_parse.c b/http_parse.c
index a5940ff..b0a6a9d 100644
--- a/http_parse.c
+++ b/http_parse.c
@@ -1306,6 +1306,7 @@ httpParseHeaders(int client, AtomPtr url,
                 do_log(L_ERROR, "Couldn't allocate condition.\n");
                 if(im) free(im);
                 if(inm) free(inm);
+                if(ifrange) free(ifrange);
             }
         } else {
             condition = NULL;
@@ -1362,6 +1363,9 @@ httpParseHeaders(int client, AtomPtr url,
     if(hbuf && hbuf != hbuf_small) free(hbuf);
     if(name) releaseAtom(name);
     if(etag) free(etag);
+    if(im) free(im);
+    if(inm) free(inm);
+    if(ifrange) free(ifrange);
     if(location) free(location);
     if(via) releaseAtom(via);
     if(expect) releaseAtom(expect);
-- 
1.8.0

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)

iEYEARECAAYFAlCk1G4ACgkQBYqIVf93VJ2MWwCdHR5FArsW6h0YH//xaCNfaXWL
agQAn1kMnoZJ9pUxHy6bTpa9gOjU8U9R
=Ib3i
-----END PGP SIGNATURE-----