From 8b5102ca835d73d5cc633f1e7b8d05b3a8082f61 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Fri, 21 Aug 2009 12:01:36 +0000 Subject: [PATCH] - Andre Guibert de Bruet pointed out a missing return code check for a strdup() that could lead to segfault if it returned NULL. I extended his suggest patch to now have Curl_retry_request() return a regular return code and better check that. --- CHANGES | 5 +++++ RELEASE-NOTES | 3 ++- lib/multi.c | 19 ++++++++++++++++--- lib/transfer.c | 24 +++++++++++++++--------- lib/transfer.h | 4 ++-- 5 files changed, 40 insertions(+), 15 deletions(-) diff --git a/CHANGES b/CHANGES index 650a36f7b..eccb1db19 100644 --- a/CHANGES +++ b/CHANGES @@ -7,6 +7,11 @@ Changelog Daniel Stenberg (21 Aug 2009) +- Andre Guibert de Bruet pointed out a missing return code check for a + strdup() that could lead to segfault if it returned NULL. I extended his + suggest patch to now have Curl_retry_request() return a regular return code + and better check that. + - Lots of good work by Krister Johansen, mostly related to pipelining: Fix SIGSEGV on free'd easy_conn when pipe unexpectedly breaks diff --git a/RELEASE-NOTES b/RELEASE-NOTES index 5af32e16a..77401bbe5 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -18,6 +18,7 @@ This release includes the following bugfixes: o SIGSEGV when pipelined pipe unexpectedly breaks o data corruption issue with re-connected transfers o use after free if we're completed but easy_conn not NULL (pipelined) + o missing strdup() return code check This release includes the following known bugs: @@ -26,6 +27,6 @@ This release includes the following known bugs: This release would not have looked like this without help, code, reports and advice from friends like these: - Karl Moerder, Kamil Dudka, Krister Johansen, + Karl Moerder, Kamil Dudka, Krister Johansen, Andre Guibert de Bruet Thanks! (and sorry if I forgot to mention someone) diff --git a/lib/multi.c b/lib/multi.c index 1099b525d..686372ad1 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -1183,7 +1183,16 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, 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; Curl_posttransfer(easy->easy_handle); drc = Curl_done(&easy->easy_conn, easy->result, FALSE); @@ -1370,9 +1379,13 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, } else if(TRUE == done) { char *newurl; - bool retry = Curl_retry_request(easy->easy_conn, &newurl); + bool retry = FALSE; followtype follow=FOLLOW_NONE; + easy->result = Curl_retry_request(easy->easy_conn, &newurl); + if(!easy->result) + retry = newurl?TRUE:FALSE; + /* call this even if the readwrite function returned error */ Curl_posttransfer(easy->easy_handle); @@ -1406,7 +1419,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, multistate(easy, CURLM_STATE_CONNECT); result = CURLM_CALL_MULTI_PERFORM; } - else + else if(newurl) /* Since we "took it", we are in charge of freeing this on failure */ free(newurl); diff --git a/lib/transfer.c b/lib/transfer.c index e5ba279d0..5bd742f47 100644 --- a/lib/transfer.c +++ b/lib/transfer.c @@ -2550,19 +2550,20 @@ Curl_reconnect_request(struct connectdata **connp) return result; } -/* Returns TRUE and sets '*url' if a request retry is wanted. +/* Returns CURLE_OK *and* sets '*url' if a request retry is wanted. NOTE: that the *url is malloc()ed. */ -bool Curl_retry_request(struct connectdata *conn, - char **url) +CURLcode Curl_retry_request(struct connectdata *conn, + char **url) { - bool retry = FALSE; struct SessionHandle *data = conn->data; + *url = NULL; + /* if we're talking upload, we can't do the checks below, unless the protocol is HTTP as when uploading over HTTP we will still get a response */ if(data->set.upload && !(conn->protocol&PROT_HTTP)) - return retry; + return CURLE_OK; if((data->req.bytecount + data->req.headerbytecount == 0) && @@ -2574,6 +2575,8 @@ bool Curl_retry_request(struct connectdata *conn, it again. Bad luck. Retry the same request on a fresh connect! */ infof(conn->data, "Connection died, retrying a fresh connect\n"); *url = strdup(conn->data->change.url); + if(!*url) + return CURLE_OUT_OF_MEMORY; conn->bits.close = TRUE; /* close this connection */ conn->bits.retry = TRUE; /* mark this as a connection we're about @@ -2581,10 +2584,8 @@ bool Curl_retry_request(struct connectdata *conn, prevent i.e HTTP transfers to return error just because nothing has been transfered! */ - retry = TRUE; } - - return retry; + return CURLE_OK; } /* @@ -2629,7 +2630,12 @@ CURLcode Curl_perform(struct SessionHandle *data) if(res == CURLE_OK) { res = Transfer(conn); /* now fetch that URL please */ if((res == CURLE_OK) || (res == CURLE_RECV_ERROR)) { - bool retry = Curl_retry_request(conn, &newurl); + bool retry = FALSE; + CURLcode rc = Curl_retry_request(conn, &newurl); + if(rc) + res = rc; + else + retry = newurl?TRUE:FALSE; if(retry) { res = CURLE_OK; diff --git a/lib/transfer.h b/lib/transfer.h index 4b39faa18..a9b1cd370 100644 --- a/lib/transfer.h +++ b/lib/transfer.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2008, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2009, Daniel Stenberg, , et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -47,7 +47,7 @@ int Curl_single_getsock(const struct connectdata *conn, CURLcode Curl_readrewind(struct connectdata *conn); CURLcode Curl_fillreadbuffer(struct connectdata *conn, int bytes, int *nreadp); CURLcode Curl_reconnect_request(struct connectdata **connp); -bool Curl_retry_request(struct connectdata *conn, char **url); +CURLcode Curl_retry_request(struct connectdata *conn, char **url); /* This sets up a forthcoming transfer */ CURLcode