On Mon, Mar 11, 2013 at 06:09:29PM +0200, Chrysovaladis Datsios wrote:
> --- a/lib/transfer.c
> +++ b/lib/transfer.c
> @@ -193,6 +193,71 @@ 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);
>
> + /* If is the last chunk, here put the trailer headers if any */
> + if((nread - hexlen) == 0) {
> + if(data->set.trailerheaders_func) {
> + struct curl_slist *trailer_headers;
> + struct curl_slist *list_head;
> +
> + /* The callback function that adds trailer header values */
> + trailer_headers = (data->set.trailerheaders_func)(data,
> + data->set.trailer_udata);
> + list_head = trailer_headers;
> + if(trailer_headers) {
> + char *ptr;

While we're here, this could be made const char *ptr (and it could be moved
into the body of the while loop).

> + size_t headers_buf_size;
> + size_t endl_len;
> + size_t data_len;
> + size_t headers_index;
> + char *trailer_headers_buf;
> +
> + endl_len = strlen(endofline_native);
> + headers_buf_size = HEADERSIZE;
> + trailer_headers_buf = malloc(headers_buf_size);
> + if(trailer_headers_buf == NULL)
> + return CURLE_BAD_FUNCTION_ARGUMENT;
> + memcpy(trailer_headers_buf, hexbuffer, hexlen);
> + headers_index = hexlen;
> +
> + while(trailer_headers) {
> + /* header name_field : header value_field */
> + ptr = strchr(trailer_headers->data, ':');
> + if(ptr) {
> + data_len = strlen(trailer_headers->data);
> + if(headers_index + data_len + 2*endl_len > headers_buf_size) {
> + size_t newsize = headers_index + data_len + 2*endl_len;
> + if(newsize > CURL_MAX_HTTP_HEADER) {
> + curl_slist_free_all(list_head);
> + list_head = NULL;
> + return CURLE_BAD_FUNCTION_ARGUMENT;

trailer_headers_buf will leak here.

> + }
> + trailer_headers_buf = realloc(trailer_headers_buf, newsize);
> + if(trailer_headers_buf == NULL) {
> + curl_slist_free_all(list_head);
> + list_head = NULL;

I wouldn't bother setting a local variable to NULL immediately before
returning. Same just above.

> + return CURLE_BAD_FUNCTION_ARGUMENT;
> + }
> + headers_buf_size = newsize;
> + }
> + memcpy(trailer_headers_buf + headers_index,
> + trailer_headers->data, data_len);
> + headers_index += data_len;
> + memcpy(trailer_headers_buf + headers_index,
> + endofline_native, endl_len);
> + headers_index += endl_len;
> + }
> + trailer_headers = trailer_headers->next;
> + }
> + nread = headers_index;
> + data->req.upload_fromhere = trailer_headers_buf;

I still don't see where trailer_headers_buf is ever freed.

> + curl_slist_free_all(list_head);
> + list_head = NULL;

I wouldn't bother with this NULL setting either, since list_head goes
out of scope here anyway.

> + }
> + }
> + /* mark this as done once this chunk is transferred */
> + data->req.upload_done = TRUE;

Moving this here changes the behaviour when the CURL_DOES_CONVERSIONS
exits due to an error. This is probably fine, but have you verified that there
aren't any unexpected effects through the error path?

> + }
> +
> /* always append ASCII CRLF to the data */
> memcpy(data->req.upload_fromhere + nread,
> endofline_network,
> @@ -215,10 +280,6 @@ CURLcode Curl_fillreadbuffer(struct connectdata *conn, int bytes, int *nreadp)
> return(res);
> #endif /* CURL_DOES_CONVERSIONS */
>
> - if((nread - hexlen) == 0)
> - /* mark this as done once this chunk is transferred */
> - data->req.upload_done = TRUE;
> -
> nread+=(int)strlen(endofline_native); /* for the added end of line */
> }
> #ifdef CURL_DOES_CONVERSIONS

>>> Dan
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-03-13