From be7faf7c0826544382165e47d8d33e716fd12622 Mon Sep 17 00:00:00 2001
From: Robert Lubos <robert.lubos@nordicsemi.no>
Date: Tue, 14 Sep 2021 17:01:32 +0200
Subject: [PATCH] net: http: Fix HTTP_DATA_FINAL notification

HTTP_DATA_FINAL was incorrectly notified in case Content Length field
was present in the HTTP respone - in such case it was set for every
response fragment, not only the last one.

Fix this by relying on `message_complete` flag instead of
`http_should_keep_alive()` function to determine whether to notify
HTTP_DATA_FINAL or not. As the HTTP parser calls the
`on_message_complete()` callback in either case (response is chunked or
not), this seems to be a more reasonable apporach to determine whether
the fragment is final or not.

Additinally, instead of calling response callback for
`on_body`/`on_message_complete` separately, call it directly from
`http_wait_data()` function, after the parsing round. This fixes the
case when headers were not reported correctly when the provided buffer
was smaller than the total headers length, resulting in corrupted data
being reported to the user.

Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
---
 subsys/net/lib/http/http_client.c | 70 ++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/subsys/net/lib/http/http_client.c b/subsys/net/lib/http/http_client.c
index 0d8f953a102..a2b9fa26ff7 100644
--- a/subsys/net/lib/http/http_client.c
+++ b/subsys/net/lib/http/http_client.c
@@ -269,28 +269,6 @@ static int on_body(struct http_parser *parser, const char *at, size_t length)
 		req->internal.response.body_start = (uint8_t *)at;
 	}
 
-	if (req->internal.response.cb) {
-		if (http_should_keep_alive(parser)) {
-			NET_DBG("Calling callback for partitioned %zd len data",
-				req->internal.response.data_len);
-
-			req->internal.response.cb(&req->internal.response,
-						  HTTP_DATA_MORE,
-						  req->internal.user_data);
-		} else {
-			NET_DBG("Calling callback for %zd len data",
-				req->internal.response.data_len);
-
-			req->internal.response.cb(&req->internal.response,
-						  HTTP_DATA_FINAL,
-						  req->internal.user_data);
-		}
-
-		/* Re-use the result buffer and start to fill it again */
-		req->internal.response.data_len = 0;
-		req->internal.response.body_start = NULL;
-	}
-
 	return 0;
 }
 
@@ -354,12 +332,6 @@ static int on_message_complete(struct http_parser *parser)
 
 	req->internal.response.message_complete = 1;
 
-	if (req->internal.response.cb) {
-		req->internal.response.cb(&req->internal.response,
-					  HTTP_DATA_FINAL,
-					  req->internal.user_data);
-	}
-
 	return 0;
 }
 
@@ -416,12 +388,21 @@ static int http_wait_data(int sock, struct http_request *req)
 
 	do {
 		received = zsock_recv(sock, req->internal.response.recv_buf + offset,
-				req->internal.response.recv_buf_len - offset,
-				0);
+				      req->internal.response.recv_buf_len - offset,
+				      0);
 		if (received == 0) {
 			/* Connection closed */
 			LOG_DBG("Connection closed");
 			ret = total_received;
+
+			if (req->internal.response.cb) {
+				NET_DBG("Calling callback for closed connection");
+
+				req->internal.response.cb(&req->internal.response,
+							  HTTP_DATA_FINAL,
+							  req->internal.user_data);
+			}
+
 			break;
 		} else if (received < 0) {
 			/* Socket error */
@@ -445,6 +426,35 @@ static int http_wait_data(int sock, struct http_request *req)
 			offset = 0;
 		}
 
+		if (req->internal.response.cb) {
+			bool notify = false;
+			enum http_final_call event;
+
+			if (req->internal.response.message_complete) {
+				NET_DBG("Calling callback for %zd len data",
+					req->internal.response.data_len);
+
+				notify = true;
+				event = HTTP_DATA_FINAL;
+			} else if (offset == 0) {
+				NET_DBG("Calling callback for partitioned %zd len data",
+					req->internal.response.data_len);
+
+				notify = true;
+				event = HTTP_DATA_MORE;
+			}
+
+			if (notify) {
+				req->internal.response.cb(&req->internal.response,
+							  event,
+							  req->internal.user_data);
+
+				/* Re-use the result buffer and start to fill it again */
+				req->internal.response.data_len = 0;
+				req->internal.response.body_start = NULL;
+			}
+		}
+
 		if (req->internal.response.message_complete) {
 			ret = total_received;
 			break;