From: Timo Sirainen <timo.sirainen@dovecot.fi>
Date: Mon, 22 Feb 2016 18:47:37 +0000 (+0200)
Subject: lib-http: Clarify http_client_request_*_payload() API and minor change to it
X-Git-Tag: 2.2.22.rc1~102
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c3a4c93;p=thirdparty%2Fdovecot%2Fcore.git

lib-http: Clarify http_client_request_*_payload() API and minor change to it

The earlier behavior was pretty confusing, and potentially could have caused
double-freeing memory in some situations. Now it's clear that req is set to NULL
always when the request is finished, regardless of whether it has any references left.

Changed http_client_request_finish_payload() to return 0 on success instead of 1.
This could have been left alone, but it's unlikely that there is any code outside
Dovecot core that calls it and this way is cleaner.
---

diff --git a/src/lib-http/http-client-request.c b/src/lib-http/http-client-request.c
index 2c985d7d13..492a22887e 100644
--- a/src/lib-http/http-client-request.c
+++ b/src/lib-http/http-client-request.c
@@ -711,14 +711,30 @@ http_client_request_continue_payload(struct http_client_request **_req,
 int http_client_request_send_payload(struct http_client_request **_req,
 	const unsigned char *data, size_t size)
 {
+	struct http_client_request *req = *_req;
+	int ret;
+
 	i_assert(data != NULL);
 
-	return http_client_request_continue_payload(_req, data, size);
+	ret = http_client_request_continue_payload(&req, data, size);
+	if (ret < 0)
+		*_req = NULL;
+	else {
+		i_assert(ret == 0);
+		i_assert(req != NULL);
+	}
+	return ret;
 }
 
 int http_client_request_finish_payload(struct http_client_request **_req)
 {
-	return http_client_request_continue_payload(_req, NULL, 0);
+	struct http_client_request *req = *_req;
+	int ret;
+
+	*_req = NULL;
+	ret = http_client_request_continue_payload(&req, NULL, 0);
+	i_assert(ret != 0);
+	return ret < 0 ? -1 : 0;
 }
 
 static void http_client_request_payload_input(struct http_client_request *req)
diff --git a/src/lib-http/http-client.h b/src/lib-http/http-client.h
index 016a05dcff..9f59c58d04 100644
--- a/src/lib-http/http-client.h
+++ b/src/lib-http/http-client.h
@@ -236,9 +236,13 @@ void http_client_request_set_destroy_callback(struct http_client_request *req,
 
 /* submits request and blocks until provided payload is sent. Multiple calls
    are allowed; payload transmission is ended with
-   http_client_request_finish_payload(). */
+   http_client_request_finish_payload(). If the sending fails, returns -1
+   and sets req=NULL to indicate that the request was freed, otherwise
+   returns 0 and req is unchanged. */
 int http_client_request_send_payload(struct http_client_request **req,
 	const unsigned char *data, size_t size);
+/* Finish sending the payload. Always frees req and sets it to NULL.
+   Returns 0 on success, -1 on error. */
 int http_client_request_finish_payload(struct http_client_request **req);
 
 void http_client_request_start_tunnel(struct http_client_request *req,
diff --git a/src/plugins/fts-solr/solr-connection.c b/src/plugins/fts-solr/solr-connection.c
index 54abf57a45..1eca106876 100644
--- a/src/plugins/fts-solr/solr-connection.c
+++ b/src/plugins/fts-solr/solr-connection.c
@@ -532,7 +532,7 @@ int solr_connection_post_end(struct solr_connection_post **_post)
 	*_post = NULL;
 
 	if (!post->failed) {
-		if (http_client_request_finish_payload(&post->http_req) <= 0 ||
+		if (http_client_request_finish_payload(&post->http_req) < 0 ||
 			conn->request_status < 0) {
 			ret = -1;
 		}