generic-library/curl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

- Lots of good work by Krister Johansen, mostly related to pipelining:

Browse Source 
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
This commit is contained in:
Daniel Stenberg 2009-08-21 07:11:20 +00:00
parent 2c4fcf2ea8
commit 1048043963
9 changed files with 213 additions and 87 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
CHANGES
View File

@ -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:

6
RELEASE-NOTES
View File

@ -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)

3
TODO-RELEASE
View File

@ -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

3
docs/KNOWN_BUGS
View File

@ -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

127
lib/multi.c
View File

@ -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,22 +1432,36 @@ 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);
/*
* 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;
}
/* post-transfer command */
easy->result = Curl_done(&easy->easy_conn, CURLE_OK, FALSE);
/* after we have DONE what we're supposed to do, go COMPLETED, and
it doesn't matter what the Curl_done() returned! */
multistate(easy, CURLM_STATE_COMPLETED);
@ -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)
{

63
lib/transfer.c
View File

@ -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. */

1
lib/transfer.h
View File

@ -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 */

88
lib/url.c
View File

@ -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. */
/*
* 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);
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;
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)

2
lib/urldata.h
View File

@ -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;