On Fri, Aug 21, 2009 at 09:35:34AM +0200, Daniel Stenberg wrote:
> On Wed, 19 Aug 2009, Andre Guibert de Bruet wrote:
>> The attached patch addresses the issue. Could it be committed upon review?
>
> Ah yes, thanks.
>
> I think the patch was a bit too simple and bailed out a little too much
> upon that error. I took this somewhat further and poked the
> Curl_retry_request() function to properly return an error code instead
> and then the change ends up somewhat larger. See my attached patch. Oh,
> and also the code in CVS changed a bit with johansen's pipelining fixes
> so this probably won't apply to anything that isn't pretty much CVS HEAD
>
> Comments anyone?

> Index: lib/multi.c
> ===================================================================
> RCS file: /cvsroot/curl/curl/lib/multi.c,v
> retrieving revision 1.202
> diff -u -r1.202 multi.c
> --- lib/multi.c 21 Aug 2009 07:11:20 -0000 1.202
> +++ lib/multi.c 21 Aug 2009 07:34:12 -0000
> @@ -1183,7 +1183,16 @@
> char *newurl;
> followtype follow=FOLLOW_NONE;
> CURLcode drc;
> - bool retry = Curl_retry_request(easy->easy_conn, &newurl);
> + bool retry = FALSE;
> +
> + drc = Curl_retry_request(easy->easy_conn, &newurl);
> + if(drc) {
> + /* a failure here pretty much implies an out of memory */
> + easy->result = drc;
> + disconnect_conn = TRUE;
> + }
> + else
> + retry = newurl?TRUE:FALSE;

I have to confess that when I first looked at this, I was confused why
it was safe to check newurl in the else case. It doesn't get
initialized when it's declared. My first thought was that we could be
checking garbage on the stack. It wasn't until I realized that the
strdup in Curl_retry_request modifies newurl even when it fails -- a
failure of strdup() sets it to NULL -- that I understood why this was
okay.

The fix looks right. It's a bit subtle and I'm under-caffienated,
though. :)

-j
Received on 2009-08-21