diff options
| -rw-r--r-- | CHANGES | 7 | ||||
| -rw-r--r-- | RELEASE-NOTES | 6 | ||||
| -rw-r--r-- | TODO-RELEASE | 3 | ||||
| -rw-r--r-- | docs/KNOWN_BUGS | 3 | ||||
| -rw-r--r-- | lib/multi.c | 127 | ||||
| -rw-r--r-- | lib/transfer.c | 63 | ||||
| -rw-r--r-- | lib/transfer.h | 1 | ||||
| -rw-r--r-- | lib/url.c | 88 | ||||
| -rw-r--r-- | lib/urldata.h | 2 |
9 files changed, 213 insertions, 87 deletions
@@ -6,6 +6,13 @@ Changelog +Daniel Stenberg (21 Aug 2009) +- Lots of good work by Krister Johansen, mostly related to pipelining: + + Fix SIGSEGV on free'd easy_conn when pipe unexpectedly breaks + Fix data corruption issue with re-connected transfers + Fix use after free if we're completed but easy_conn not NULL + Kamil Dudka (13 Aug 2009) - Changed NSS code to not ignore the value of ssl.verifyhost and produce more verbose error messages. Originally reported at: diff --git a/RELEASE-NOTES b/RELEASE-NOTES index f38ff7091..5af32e16a 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -14,6 +14,10 @@ This release includes the following changes: This release includes the following bugfixes: o The windows makefiles work again + o libcurl-NSS acknowledges verifyhost + 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) This release includes the following known bugs: @@ -22,6 +26,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 + Karl Moerder, Kamil Dudka, Krister Johansen, Thanks! (and sorry if I forgot to mention someone) diff --git a/TODO-RELEASE b/TODO-RELEASE index 4897358cf..5c4add331 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -10,9 +10,6 @@ To be addressed in 7.19.7 (planned release: October 2009) 254 - Problem re-using easy handle after call to curl_multi_remove_handle http://curl.haxx.se/mail/lib-2009-07/0249.html -255 - debugging a crash in Curl_pgrsTime/checkPendPipeline? - http://curl.haxx.se/mail/lib-2009-08/0066.html - 256 - "More questions about ares behavior" http://curl.haxx.se/mail/lib-2009-08/0012.html diff --git a/docs/KNOWN_BUGS b/docs/KNOWN_BUGS index 474991d90..35ca80a84 100644 --- a/docs/KNOWN_BUGS +++ b/docs/KNOWN_BUGS @@ -12,9 +12,6 @@ may have been fixed since this was written! 70. Problem re-using easy handle after call to curl_multi_remove_handle http://curl.haxx.se/mail/lib-2009-07/0249.html -69. debugging a crash in Curl_pgrsTime/checkPendPipeline? - http://curl.haxx.se/mail/lib-2009-08/0066.html - 68. "More questions about ares behavior". http://curl.haxx.se/mail/lib-2009-08/0012.html diff --git a/lib/multi.c b/lib/multi.c index 0f75c2d72..1099b525d 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -192,7 +192,9 @@ static int update_timer(struct Curl_multi *multi); static CURLcode addHandleToSendOrPendPipeline(struct SessionHandle *handle, struct connectdata *conn); static int checkPendPipeline(struct connectdata *conn); -static void moveHandleFromSendToRecvPipeline(struct SessionHandle *habdle, +static void moveHandleFromSendToRecvPipeline(struct SessionHandle *handle, + struct connectdata *conn); +static void moveHandleFromRecvToDonePipeline(struct SessionHandle *handle, struct connectdata *conn); static bool isHandleAtHead(struct SessionHandle *handle, struct curl_llist *pipeline); @@ -233,14 +235,16 @@ static void multistate(struct Curl_one_easy *easy, CURLMstate state) easy->state = state; #ifdef DEBUGBUILD - if(easy->state > CURLM_STATE_CONNECT && - easy->state < CURLM_STATE_COMPLETED) - connectindex = easy->easy_conn->connectindex; + if(easy->easy_conn) { + if(easy->state > CURLM_STATE_CONNECT && + easy->state < CURLM_STATE_COMPLETED) + connectindex = easy->easy_conn->connectindex; - infof(easy->easy_handle, - "STATE: %s => %s handle %p; (connection #%ld) \n", - statename[oldstate], statename[easy->state], - (char *)easy, connectindex); + infof(easy->easy_handle, + "STATE: %s => %s handle %p; (connection #%ld) \n", + statename[oldstate], statename[easy->state], + (char *)easy, connectindex); + } #endif if(state == CURLM_STATE_COMPLETED) /* changing to COMPLETED means there's one less easy handle 'alive' */ @@ -925,7 +929,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, break; } - if(easy->state > CURLM_STATE_CONNECT && + if(easy->easy_conn && easy->state > CURLM_STATE_CONNECT && easy->state < CURLM_STATE_COMPLETED) /* Make sure we set the connection's current owner */ easy->easy_conn->data = easy->easy_handle; @@ -1149,7 +1153,6 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, &dophase_done); if(CURLE_OK == easy->result) { - if(!dophase_done) { /* DO was not completed in one function call, we must continue DOING... */ @@ -1170,6 +1173,49 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, result = CURLM_CALL_MULTI_PERFORM; } } + else if ((CURLE_SEND_ERROR == easy->result) && + easy->easy_conn->bits.reuse) { + /* + * In this situation, a connection that we were trying to use + * may have unexpectedly died. If possible, send the connection + * back to the CONNECT phase so we can try again. + */ + char *newurl; + followtype follow=FOLLOW_NONE; + CURLcode drc; + bool retry = Curl_retry_request(easy->easy_conn, &newurl); + + Curl_posttransfer(easy->easy_handle); + drc = Curl_done(&easy->easy_conn, easy->result, FALSE); + + /* When set to retry the connection, we must to go back to + * the CONNECT state */ + if(retry) { + if ((drc == CURLE_OK) || (drc == CURLE_SEND_ERROR)) { + follow = FOLLOW_RETRY; + drc = Curl_follow(easy->easy_handle, newurl, follow); + if(drc == CURLE_OK) { + multistate(easy, CURLM_STATE_CONNECT); + result = CURLM_CALL_MULTI_PERFORM; + easy->result = CURLE_OK; + } + else { + /* Follow failed */ + easy->result = drc; + free(newurl); + } + } + else { + /* done didn't return OK or SEND_ERROR */ + easy->result = drc; + free(newurl); + } + } + else { + /* Have error handler disconnect conn if we can't retry */ + disconnect_conn = TRUE; + } + } else { /* failure detected */ Curl_posttransfer(easy->easy_handle); @@ -1331,8 +1377,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, Curl_posttransfer(easy->easy_handle); /* we're no longer receving */ - Curl_removeHandleFromPipeline(easy->easy_handle, - easy->easy_conn->recv_pipe); + moveHandleFromRecvToDonePipeline(easy->easy_handle, + easy->easy_conn); /* expire the new receiving pipeline head */ if(easy->easy_conn->recv_pipe->head) @@ -1386,21 +1432,35 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, break; case CURLM_STATE_DONE: - /* Remove ourselves from the receive pipeline */ - Curl_removeHandleFromPipeline(easy->easy_handle, - easy->easy_conn->recv_pipe); - /* Check if we can move pending requests to send pipe */ - checkPendPipeline(easy->easy_conn); - if(easy->easy_conn->bits.stream_was_rewound) { - /* This request read past its response boundary so we quickly let the - other requests consume those bytes since there is no guarantee that - the socket will become active again */ - result = CURLM_CALL_MULTI_PERFORM; - } + if(easy->easy_conn) { + /* Remove ourselves from the receive and done pipelines. Handle + should be on one of these lists, depending upon how we got here. */ + Curl_removeHandleFromPipeline(easy->easy_handle, + easy->easy_conn->recv_pipe); + Curl_removeHandleFromPipeline(easy->easy_handle, + easy->easy_conn->done_pipe); + /* Check if we can move pending requests to send pipe */ + checkPendPipeline(easy->easy_conn); + + if(easy->easy_conn->bits.stream_was_rewound) { + /* This request read past its response boundary so we quickly let + the other requests consume those bytes since there is no + guarantee that the socket will become active again */ + result = CURLM_CALL_MULTI_PERFORM; + } - /* post-transfer command */ - easy->result = Curl_done(&easy->easy_conn, CURLE_OK, FALSE); + /* post-transfer command */ + easy->result = Curl_done(&easy->easy_conn, CURLE_OK, FALSE); + /* + * If there are other handles on the pipeline, Curl_done won't set + * easy_conn to NULL. In such a case, curl_multi_remove_handle() can + * access free'd data, if the connection is free'd and the handle + * removed before we perform the processing in CURLM_STATE_COMPLETED + */ + if (easy->easy_conn) + easy->easy_conn = NULL; + } /* after we have DONE what we're supposed to do, go COMPLETED, and it doesn't matter what the Curl_done() returned! */ @@ -1443,6 +1503,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, easy->easy_conn->send_pipe); Curl_removeHandleFromPipeline(easy->easy_handle, easy->easy_conn->recv_pipe); + Curl_removeHandleFromPipeline(easy->easy_handle, + easy->easy_conn->done_pipe); /* Check if we can move pending requests to send pipe */ checkPendPipeline(easy->easy_conn); } @@ -2173,6 +2235,21 @@ static void moveHandleFromSendToRecvPipeline(struct SessionHandle *handle, } } +static void moveHandleFromRecvToDonePipeline(struct SessionHandle *handle, + struct connectdata *conn) +{ + struct curl_llist_element *curr; + + curr = conn->recv_pipe->head; + while(curr) { + if(curr->ptr == handle) { + Curl_llist_move(conn->recv_pipe, curr, + conn->done_pipe, conn->done_pipe->tail); + break; + } + curr = curr->next; + } +} static bool isHandleAtHead(struct SessionHandle *handle, struct curl_llist *pipeline) { diff --git a/lib/transfer.c b/lib/transfer.c index d59b3f500..e5ba279d0 100644 --- a/lib/transfer.c +++ b/lib/transfer.c @@ -176,7 +176,7 @@ CURLcode Curl_fillreadbuffer(struct connectdata *conn, int bytes, int *nreadp) } if(!data->req.forbidchunk && data->req.upload_chunky) { - /* if chunked Transfer-Encoding + /* if chunked Transfer-Encoding * build chunk: * * <HEX SIZE> CRLF @@ -217,9 +217,9 @@ CURLcode Curl_fillreadbuffer(struct connectdata *conn, int bytes, int *nreadp) /* copy the prefix to the buffer, leaving out the NUL */ memcpy(data->req.upload_fromhere, hexbuffer, hexlen); - /* always append ASCII CRLF to the data */ - memcpy(data->req.upload_fromhere + nread, - endofline_network, + /* always append ASCII CRLF to the data */ + memcpy(data->req.upload_fromhere + nread, + endofline_network, strlen(endofline_network)); #ifdef CURL_DOES_CONVERSIONS @@ -2495,6 +2495,61 @@ connect_host(struct SessionHandle *data, return res; } +CURLcode +Curl_reconnect_request(struct connectdata **connp) +{ + CURLcode result = CURLE_OK; + struct connectdata *conn = *connp; + struct SessionHandle *data = conn->data; + + /* This was a re-use of a connection and we got a write error in the + * DO-phase. Then we DISCONNECT this connection and have another attempt to + * CONNECT and then DO again! The retry cannot possibly find another + * connection to re-use, since we only keep one possible connection for + * each. */ + + infof(data, "Re-used connection seems dead, get a new one\n"); + + conn->bits.close = TRUE; /* enforce close of this connection */ + result = Curl_done(&conn, result, FALSE); /* we are so done with this */ + + /* conn may no longer be a good pointer */ + + /* + * According to bug report #1330310. We need to check for CURLE_SEND_ERROR + * here as well. I figure this could happen when the request failed on a FTP + * connection and thus Curl_done() itself tried to use the connection + * (again). Slight Lack of feedback in the report, but I don't think this + * extra check can do much harm. + */ + if((CURLE_OK == result) || (CURLE_SEND_ERROR == result)) { + bool async; + bool protocol_done = TRUE; + + /* Now, redo the connect and get a new connection */ + result = Curl_connect(data, connp, &async, &protocol_done); + if(CURLE_OK == result) { + /* We have connected or sent away a name resolve query fine */ + + conn = *connp; /* setup conn to again point to something nice */ + if(async) { + /* Now, if async is TRUE here, we need to wait for the name + to resolve */ + result = Curl_wait_for_resolv(conn, NULL); + if(result) + return result; + + /* Resolved, continue with the connection */ + result = Curl_async_resolved(conn, &protocol_done); + if(result) + return result; + } + } + } + + return result; +} + /* Returns TRUE and sets '*url' if a request retry is wanted. NOTE: that the *url is malloc()ed. */ diff --git a/lib/transfer.h b/lib/transfer.h index aad82ebaf..4b39faa18 100644 --- a/lib/transfer.h +++ b/lib/transfer.h @@ -46,6 +46,7 @@ int Curl_single_getsock(const struct connectdata *conn, int numsocks); 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); /* This sets up a forthcoming transfer */ @@ -146,7 +146,7 @@ void idn_free (void *ptr); /* prototype from idn-free.h, not provided by /* Local static prototypes */ static long ConnectionKillOne(struct SessionHandle *data); static void conn_free(struct connectdata *conn); -static void signalPipeClose(struct curl_llist *pipeline); +static void signalPipeClose(struct curl_llist *pipeline, bool pipe_broke); #ifdef CURL_DISABLE_VERBOSE_STRINGS #define verboseconnect(x) do { } while (0) @@ -420,6 +420,16 @@ CURLcode Curl_close(struct SessionHandle *data) } } } + pipeline = connptr->done_pipe; + if(pipeline) { + for (curr = pipeline->head; curr; curr=curr->next) { + if(data == (struct SessionHandle *) curr->ptr) { + fprintf(stderr, + "MAJOR problem we %p are still in done pipe for %p done %d\n", + data, connptr, (int)connptr->bits.done); + } + } + } pipeline = connptr->pend_pipe; if(pipeline) { for (curr = pipeline->head; curr; curr=curr->next) { @@ -2316,6 +2326,7 @@ static void conn_free(struct connectdata *conn) Curl_llist_destroy(conn->send_pipe, NULL); Curl_llist_destroy(conn->recv_pipe, NULL); Curl_llist_destroy(conn->pend_pipe, NULL); + Curl_llist_destroy(conn->done_pipe, NULL); /* possible left-overs from the async name resolvers */ #if defined(USE_ARES) @@ -2415,9 +2426,10 @@ CURLcode Curl_disconnect(struct connectdata *conn) /* Indicate to all handles on the pipe that we're dead */ if(Curl_isPipeliningEnabled(data)) { - signalPipeClose(conn->send_pipe); - signalPipeClose(conn->recv_pipe); - signalPipeClose(conn->pend_pipe); + signalPipeClose(conn->send_pipe, TRUE); + signalPipeClose(conn->recv_pipe, TRUE); + signalPipeClose(conn->pend_pipe, TRUE); + signalPipeClose(conn->done_pipe, FALSE); } conn_free(conn); @@ -2535,9 +2547,10 @@ void Curl_getoff_all_pipelines(struct SessionHandle *data, if(Curl_removeHandleFromPipeline(data, conn->send_pipe) && send_head) conn->writechannel_inuse = FALSE; Curl_removeHandleFromPipeline(data, conn->pend_pipe); + Curl_removeHandleFromPipeline(data, conn->done_pipe); } -static void signalPipeClose(struct curl_llist *pipeline) +static void signalPipeClose(struct curl_llist *pipeline, bool pipe_broke) { struct curl_llist_element *curr; @@ -2556,7 +2569,8 @@ static void signalPipeClose(struct curl_llist *pipeline) } #endif - data->state.pipe_broke = TRUE; + if (pipe_broke) + data->state.pipe_broke = TRUE; Curl_multi_handlePipeBreak(data); Curl_llist_remove(pipeline, curr, NULL); curr = next; @@ -4217,6 +4231,7 @@ static void reuse_conn(struct connectdata *old_conn, Curl_llist_destroy(old_conn->send_pipe, NULL); Curl_llist_destroy(old_conn->recv_pipe, NULL); Curl_llist_destroy(old_conn->pend_pipe, NULL); + Curl_llist_destroy(old_conn->done_pipe, NULL); Curl_safefree(old_conn->master_buffer); } @@ -4319,7 +4334,9 @@ static CURLcode create_conn(struct SessionHandle *data, conn->send_pipe = Curl_llist_alloc((curl_llist_dtor) llist_dtor); conn->recv_pipe = Curl_llist_alloc((curl_llist_dtor) llist_dtor); conn->pend_pipe = Curl_llist_alloc((curl_llist_dtor) llist_dtor); - if(!conn->send_pipe || !conn->recv_pipe || !conn->pend_pipe) + conn->done_pipe = Curl_llist_alloc((curl_llist_dtor) llist_dtor); + if(!conn->send_pipe || !conn->recv_pipe || !conn->pend_pipe || + !conn->done_pipe) return CURLE_OUT_OF_MEMORY; /* This initing continues below, see the comment "Continue connectdata @@ -5001,53 +5018,22 @@ CURLcode Curl_do(struct connectdata **connp, bool *done) /* This was formerly done in transfer.c, but we better do it here */ if((CURLE_SEND_ERROR == result) && conn->bits.reuse) { - /* This was a re-use of a connection and we got a write error in the - * DO-phase. Then we DISCONNECT this connection and have another attempt - * to CONNECT and then DO again! The retry cannot possibly find another - * connection to re-use, since we only keep one possible connection for - * each. */ - - infof(data, "Re-used connection seems dead, get a new one\n"); - - conn->bits.close = TRUE; /* enforce close of this connection */ - result = Curl_done(&conn, result, FALSE); /* we are so done with this */ - - /* conn may no longer be a good pointer */ + /* + * If the connection is using an easy handle, call reconnect + * to re-establish the connection. Otherwise, let the multi logic + * figure out how to re-establish the connection. + */ + if(!data->multi) { + result = Curl_reconnect_request(connp); - /* - * According to bug report #1330310. We need to check for - * CURLE_SEND_ERROR here as well. I figure this could happen when the - * request failed on a FTP connection and thus Curl_done() itself tried - * to use the connection (again). Slight Lack of feedback in the report, - * but I don't think this extra check can do much harm. - */ - if((CURLE_OK == result) || (CURLE_SEND_ERROR == result)) { - bool async; - bool protocol_done = TRUE; - - /* Now, redo the connect and get a new connection */ - result = Curl_connect(data, connp, &async, &protocol_done); - if(CURLE_OK == result) { - /* We have connected or sent away a name resolve query fine */ - - conn = *connp; /* setup conn to again point to something nice */ - if(async) { - /* Now, if async is TRUE here, we need to wait for the name - to resolve */ - result = Curl_wait_for_resolv(conn, NULL); - if(result) - return result; - - /* Resolved, continue with the connection */ - result = Curl_async_resolved(conn, &protocol_done); - if(result) - return result; + if(result == CURLE_OK) { + /* ... finally back to actually retry the DO phase */ + result = conn->handler->do_it(conn, done); } - - /* ... finally back to actually retry the DO phase */ - result = conn->handler->do_it(conn, done); } - } + else { + return result; + } } if((result == CURLE_OK) && *done) diff --git a/lib/urldata.h b/lib/urldata.h index a48fc91b7..1ee6637d2 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -1028,6 +1028,8 @@ struct connectdata { their responses on this pipeline */ struct curl_llist *pend_pipe; /* List of pending handles on this pipeline */ + struct curl_llist *done_pipe; /* Handles that are finished, but + still reference this connectdata */ #define MAX_PIPELINE_LENGTH 5 char* master_buffer; /* The master buffer allocated on-demand; |