From c4e6968127e876b01e5e0b4b7cdbc49d5267530c Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Thu, 28 May 2020 18:30:47 +0200 Subject: [PATCH] url: alloc the download buffer at transfer start ... and free it as soon as the transfer is done. It removes the extra alloc when a new size is set with setopt() and reduces memory for unused easy handles. In addition: the closure_handle now doesn't use an allocated buffer at all but the smallest supported size as a stack based one. Closes #5472 --- lib/conncache.c | 6 ++++ lib/easy.c | 17 ------------ lib/http2.c | 9 ++++-- lib/multi.c | 22 ++++++++++++++- lib/setopt.c | 2 +- lib/transfer.c | 14 +++++----- lib/url.c | 32 ++++++--------------- lib/urldata.h | 1 - tests/data/test509 | 5 +--- tests/libtest/lib509.c | 63 ++++++++++-------------------------------- 10 files changed, 66 insertions(+), 105 deletions(-) diff --git a/lib/conncache.c b/lib/conncache.c index 95fcea6f3..4474c28b4 100644 --- a/lib/conncache.c +++ b/lib/conncache.c @@ -531,6 +531,11 @@ Curl_conncache_extract_oldest(struct Curl_easy *data) void Curl_conncache_close_all_connections(struct conncache *connc) { struct connectdata *conn; + char buffer[READBUFFER_MIN]; + if(!connc->closure_handle) + return; + connc->closure_handle->state.buffer = buffer; + connc->closure_handle->set.buffer_size = READBUFFER_MIN; conn = conncache_find_first_connection(connc); while(conn) { @@ -547,6 +552,7 @@ void Curl_conncache_close_all_connections(struct conncache *connc) conn = conncache_find_first_connection(connc); } + connc->closure_handle->state.buffer = NULL; if(connc->closure_handle) { SIGPIPE_VARIABLE(pipe_st); sigpipe_ignore(connc->closure_handle, &pipe_st); diff --git a/lib/easy.c b/lib/easy.c index 1deedb265..f58c4521a 100644 --- a/lib/easy.c +++ b/lib/easy.c @@ -829,9 +829,6 @@ struct Curl_easy *curl_easy_duphandle(struct Curl_easy *data) * the likeliness of us forgetting to init a buffer here in the future. */ outcurl->set.buffer_size = data->set.buffer_size; - outcurl->state.buffer = malloc(outcurl->set.buffer_size + 1); - if(!outcurl->state.buffer) - goto fail; /* copy all userdefined values */ if(dupset(outcurl, data)) @@ -947,8 +944,6 @@ struct Curl_easy *curl_easy_duphandle(struct Curl_easy *data) */ void curl_easy_reset(struct Curl_easy *data) { - long old_buffer_size = data->set.buffer_size; - Curl_free_request_state(data); /* zero out UserDefined data: */ @@ -972,18 +967,6 @@ void curl_easy_reset(struct Curl_easy *data) #if !defined(CURL_DISABLE_HTTP) && !defined(CURL_DISABLE_CRYPTO_AUTH) Curl_http_auth_cleanup_digest(data); #endif - - /* resize receive buffer */ - if(old_buffer_size != data->set.buffer_size) { - char *newbuff = realloc(data->state.buffer, data->set.buffer_size + 1); - if(!newbuff) { - DEBUGF(fprintf(stderr, "Error: realloc of buffer failed\n")); - /* nothing we can do here except use the old size */ - data->set.buffer_size = old_buffer_size; - } - else - data->state.buffer = newbuff; - } } /* diff --git a/lib/http2.c b/lib/http2.c index e4733c94b..7d56e2280 100644 --- a/lib/http2.c +++ b/lib/http2.c @@ -258,15 +258,14 @@ static unsigned int http2_conncheck(struct connectdata *check, void Curl_http2_setup_req(struct Curl_easy *data) { struct HTTP *http = data->req.protop; - http->bodystarted = FALSE; http->status_code = -1; http->pausedata = NULL; http->pauselen = 0; http->closed = FALSE; http->close_handled = FALSE; - http->mem = data->state.buffer; - http->len = data->set.buffer_size; + http->mem = NULL; + http->len = 0; http->memlen = 0; } @@ -2103,6 +2102,8 @@ CURLcode Curl_http2_setup(struct connectdata *conn) struct http_conn *httpc = &conn->proto.httpc; struct HTTP *stream = conn->data->req.protop; + DEBUGASSERT(conn->data->state.buffer); + stream->stream_id = -1; Curl_dyn_init(&stream->header_recvbuf, DYN_H2_HEADERS); @@ -2126,6 +2127,8 @@ CURLcode Curl_http2_setup(struct connectdata *conn) stream->upload_left = 0; stream->upload_mem = NULL; stream->upload_len = 0; + stream->mem = conn->data->state.buffer; + stream->len = conn->data->set.buffer_size; httpc->inbuflen = 0; httpc->nread_inbuf = 0; diff --git a/lib/multi.c b/lib/multi.c index 75b0357c3..b753b1a61 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -678,6 +678,7 @@ static CURLcode multi_done(struct Curl_easy *data, data->state.lastconnect = NULL; } + Curl_safefree(data->state.buffer); Curl_free_request_state(data); return result; } @@ -1522,6 +1523,20 @@ static CURLcode protocol_connect(struct connectdata *conn, return result; /* pass back status */ } +/* + * preconnect() is called immediately before a connect starts. When a redirect + * is followed, this is then called multiple times during a single transfer. + */ +static CURLcode preconnect(struct Curl_easy *data) +{ + if(!data->state.buffer) { + data->state.buffer = malloc(data->set.buffer_size + 1); + if(!data->state.buffer) + return CURLE_OUT_OF_MEMORY; + } + return CURLE_OK; +} + static CURLMcode multi_runsingle(struct Curl_multi *multi, struct curltime now, @@ -1629,6 +1644,11 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, case CURLM_STATE_CONNECT: /* Connect. We want to get a connection identifier filled in. */ + /* init this transfer. */ + result = preconnect(data); + if(result) + break; + Curl_pgrsTime(data, TIMER_STARTSINGLE); if(data->set.timeout) Curl_expire(data, data->set.timeout, EXPIRE_TIMEOUT); @@ -2058,7 +2078,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, char *newurl = NULL; bool retry = FALSE; bool comeback = FALSE; - + DEBUGASSERT(data->state.buffer); /* check if over send speed */ send_timeout_ms = 0; if(data->set.max_send_speed > 0) diff --git a/lib/setopt.c b/lib/setopt.c index 72704127c..2ddaeef7d 100644 --- a/lib/setopt.c +++ b/lib/setopt.c @@ -2076,7 +2076,7 @@ CURLcode Curl_vsetopt(struct Curl_easy *data, CURLoption option, va_list param) arg = READBUFFER_MIN; /* Resize if new size */ - if(arg != data->set.buffer_size) { + if((arg != data->set.buffer_size) && data->state.buffer) { char *newbuff = realloc(data->state.buffer, arg + 1); if(!newbuff) { DEBUGF(fprintf(stderr, "Error: realloc of buffer failed\n")); diff --git a/lib/transfer.c b/lib/transfer.c index dc43cf6ce..ea337ea1d 100644 --- a/lib/transfer.c +++ b/lib/transfer.c @@ -558,6 +558,8 @@ static CURLcode readwrite_data(struct Curl_easy *data, size_t excess = 0; /* excess bytes read */ bool readmore = FALSE; /* used by RTP to signal for more data */ int maxloops = 100; + char *buf = data->state.buffer; + DEBUGASSERT(buf); *done = FALSE; *comeback = FALSE; @@ -592,7 +594,7 @@ static CURLcode readwrite_data(struct Curl_easy *data, if(bytestoread) { /* receive data from the network! */ - result = Curl_read(conn, conn->sockfd, k->buf, bytestoread, &nread); + result = Curl_read(conn, conn->sockfd, buf, bytestoread, &nread); /* read would've blocked */ if(CURLE_AGAIN == result) @@ -619,9 +621,8 @@ static CURLcode readwrite_data(struct Curl_easy *data, /* indicates data of zero size, i.e. empty file */ is_empty_data = ((nread == 0) && (k->bodywrites == 0)) ? TRUE : FALSE; - /* NUL terminate, allowing string ops to be used */ if(0 < nread || is_empty_data) { - k->buf[nread] = 0; + buf[nread] = 0; } else { /* if we receive 0 or less here, either the http2 stream is closed or the @@ -638,7 +639,7 @@ static CURLcode readwrite_data(struct Curl_easy *data, /* Default buffer to use when we write the buffer, it may be changed in the flow below before the actual storing is done. */ - k->str = k->buf; + k->str = buf; if(conn->handler->readwrite) { result = conn->handler->readwrite(data, conn, &nread, &readmore); @@ -905,10 +906,10 @@ static CURLcode readwrite_data(struct Curl_easy *data, /* Parse the excess data */ k->str += nread; - if(&k->str[excess] > &k->buf[data->set.buffer_size]) { + if(&k->str[excess] > &buf[data->set.buffer_size]) { /* the excess amount was too excessive(!), make sure it doesn't read out of buffer */ - excess = &k->buf[data->set.buffer_size] - k->str; + excess = &buf[data->set.buffer_size] - k->str; } nread = (ssize_t)excess; @@ -1467,7 +1468,6 @@ CURLcode Curl_pretransfer(struct Curl_easy *data) data->state.authhost.want = data->set.httpauth; data->state.authproxy.want = data->set.proxyauth; Curl_safefree(data->info.wouldredirect); - data->info.wouldredirect = NULL; if(data->set.httpreq == HTTPREQ_PUT) data->state.infilesize = data->set.filesize; diff --git a/lib/url.c b/lib/url.c index 5114deaea..578844bae 100644 --- a/lib/url.c +++ b/lib/url.c @@ -610,31 +610,21 @@ CURLcode Curl_open(struct Curl_easy **curl) return result; } - /* We do some initial setup here, all those fields that can't be just 0 */ + result = Curl_init_userdefined(data); + if(!result) { + Curl_dyn_init(&data->state.headerb, CURL_MAX_HTTP_HEADER); + Curl_convert_init(data); + Curl_initinfo(data); - data->state.buffer = malloc(READBUFFER_SIZE + 1); - if(!data->state.buffer) { - DEBUGF(fprintf(stderr, "Error: malloc of buffer failed\n")); - result = CURLE_OUT_OF_MEMORY; - } - else { - result = Curl_init_userdefined(data); - if(!result) { - Curl_dyn_init(&data->state.headerb, CURL_MAX_HTTP_HEADER); - Curl_convert_init(data); - Curl_initinfo(data); + /* most recent connection is not yet defined */ + data->state.lastconnect = NULL; - /* most recent connection is not yet defined */ - data->state.lastconnect = NULL; - - data->progress.flags |= PGRS_HIDE; - data->state.current_speed = -1; /* init to negative == impossible */ - } + data->progress.flags |= PGRS_HIDE; + data->state.current_speed = -1; /* init to negative == impossible */ } if(result) { Curl_resolver_cleanup(data->state.resolver); - free(data->state.buffer); Curl_dyn_free(&data->state.headerb); Curl_freeset(data); free(data); @@ -3973,14 +3963,10 @@ CURLcode Curl_init_do(struct Curl_easy *data, struct connectdata *conn) k->start = Curl_now(); /* start time */ k->now = k->start; /* current time is now */ k->header = TRUE; /* assume header */ - k->bytecount = 0; - - k->buf = data->state.buffer; k->ignorebody = FALSE; Curl_speedinit(data); - Curl_pgrsSetUploadCounter(data, 0); Curl_pgrsSetDownloadCounter(data, 0); diff --git a/lib/urldata.h b/lib/urldata.h index 20ce5ed10..0d756f18e 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -627,7 +627,6 @@ struct SingleRequest { struct contenc_writer *writer_stack; time_t timeofdoc; long bodywrites; - char *buf; int keepon; char *location; /* This points to an allocated version of the Location: header data */ diff --git a/tests/data/test509 b/tests/data/test509 index 5de1599e1..0e0dd212b 100644 --- a/tests/data/test509 +++ b/tests/data/test509 @@ -34,10 +34,7 @@ nothing # Verify data after the test has been "shot" -seen custom_calloc() -seen custom_malloc() -seen custom_realloc() -seen custom_free() +Callbacks were invoked! diff --git a/tests/libtest/lib509.c b/tests/libtest/lib509.c index e8e803ffc..1fb2d3445 100644 --- a/tests/libtest/lib509.c +++ b/tests/libtest/lib509.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2019, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2020, 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 @@ -34,70 +34,35 @@ * memory callbacks which should be calling 'the real thing'. */ -/* -#include "memdebug.h" -*/ +static int seen; -static int seen_malloc = 0; -static int seen_free = 0; -static int seen_realloc = 0; -static int seen_strdup = 0; -static int seen_calloc = 0; - -void *custom_malloc(size_t size); -void custom_free(void *ptr); -void *custom_realloc(void *ptr, size_t size); -char *custom_strdup(const char *ptr); -void *custom_calloc(size_t nmemb, size_t size); - - -void *custom_calloc(size_t nmemb, size_t size) +static void *custom_calloc(size_t nmemb, size_t size) { - if(!seen_calloc) { - printf("seen custom_calloc()\n"); - seen_calloc = 1; - } + seen++; return (calloc)(nmemb, size); } -void *custom_malloc(size_t size) +static void *custom_malloc(size_t size) { - if(!seen_malloc && seen_calloc) { - printf("seen custom_malloc()\n"); - seen_malloc = 1; - } + seen++; return (malloc)(size); } -char *custom_strdup(const char *ptr) +static char *custom_strdup(const char *ptr) { - if(!seen_strdup && seen_malloc) { - /* currently (2013.03.13), memory tracking enabled builds do not call - the strdup callback, in this case malloc callback and memcpy are used - instead. If some day this is changed the following printf() should be - uncommented, and a line added to test definition. - printf("seen custom_strdup()\n"); - */ - seen_strdup = 1; - } + seen++; return (strdup)(ptr); } -void *custom_realloc(void *ptr, size_t size) +static void *custom_realloc(void *ptr, size_t size) { - if(!seen_realloc && seen_malloc) { - printf("seen custom_realloc()\n"); - seen_realloc = 1; - } + seen++; return (realloc)(ptr, size); } -void custom_free(void *ptr) +static void custom_free(void *ptr) { - if(!seen_free && seen_realloc) { - printf("seen custom_free()\n"); - seen_free = 1; - } + seen++; (free)(ptr); } @@ -110,7 +75,6 @@ int test(char *URL) CURL *curl; int asize; char *str = NULL; - (void)URL; res = curl_global_init_mem(CURL_GLOBAL_ALL, @@ -136,6 +100,9 @@ int test(char *URL) asize = (int)sizeof(a); str = curl_easy_escape(curl, (char *)a, asize); /* uses realloc() */ + if(seen) + printf("Callbacks were invoked!\n"); + test_cleanup: if(str)