From: Tomas Krizek <tomas.krizek@nic.cz>
Date: Tue, 30 Mar 2021 17:24:08 +0000 (+0200)
Subject: daemon/http: fix memleak if http_write_pkt() fails
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f9757a27b53f5855aec7a93c7cd4c6343c3e431d;p=thirdparty%2Fknot-resolver.git

daemon/http: fix memleak if http_write_pkt() fails

This can happen for example when we want to send an answer, but the
http stream (or the connection?) is already closed.

Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5ad2445459 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x55c0db3fc442 in http_write_pkt ../daemon/http.c:610
    #2 0x55c0db3fc882 in http_write ../daemon/http.c:651
    #3 0x55c0db3e9bb1 in qr_task_send ../daemon/worker.c:700
    #4 0x55c0db3ee86c in qr_task_finalize ../daemon/worker.c:1321
    #5 0x55c0db3f0123 in qr_task_step ../daemon/worker.c:1633
    #6 0x55c0db3f0982 in worker_submit ../daemon/worker.c:1755
    #7 0x55c0db3d992a in session_wirebuf_process ../daemon/session.c:759
    #8 0x55c0db3c5f01 in udp_recv ../daemon/io.c:89
    #9 0x7f5ad22b0e0e  (/usr/lib/libuv.so.1+0x20e0e)
---

diff --git a/daemon/http.c b/daemon/http.c
index f72465f65..a743e7f09 100644
--- a/daemon/http.c
+++ b/daemon/http.c
@@ -1,4 +1,5 @@
 /*
+ *
  * Copyright (C) 2020 CZ.NIC, z.s.p.o
  *
  * Initial Author: Jan Hák <jan.hak@nic.cz>
@@ -417,7 +418,6 @@ static void on_pkt_write(struct http_data *data, int status)
 		return;
 
 	data->on_write(data->req, status);
-
 	free(data);
 }
 
@@ -576,22 +576,33 @@ static int http_send_response(nghttp2_session *h2, int32_t stream_id,
 		MAKE_NV("cache-control", 13, max_age, max_age_len),
 	};
 
-	ret = nghttp2_submit_response(h2, stream_id, hdrs, sizeof(hdrs)/sizeof(*hdrs), prov);
+	ret = nghttp2_session_set_stream_user_data(h2, stream_id, (void*)data);
 	if (ret != 0) {
-		kr_log_verbose("[http] nghttp2_submit_response failed: %s\n", nghttp2_strerror(ret));
+		kr_log_verbose("[http] failed to set stream user data: %s\n", nghttp2_strerror(ret));
+		free(data);
 		return kr_error(EIO);
 	}
 
-	ret = nghttp2_session_set_stream_user_data(h2, stream_id, (void*)data);
+	ret = nghttp2_submit_response(h2, stream_id, hdrs, sizeof(hdrs)/sizeof(*hdrs), prov);
 	if (ret != 0) {
-		kr_log_verbose("[http] failed to set stream user data: %s\n", nghttp2_strerror(ret));
+		kr_log_verbose("[http] nghttp2_submit_response failed: %s\n", nghttp2_strerror(ret));
+		nghttp2_session_set_stream_user_data(h2, stream_id, NULL);  // just in case
+		free(data);
 		return kr_error(EIO);
 	}
 
 	ret = nghttp2_session_send(h2);
 	if(ret < 0) {
 		kr_log_verbose("[http] nghttp2_session_send failed: %s\n", nghttp2_strerror(ret));
-		return kr_error(EIO);
+
+		/* At this point, there was an error in some nghttp2 callback. The on_pkt_write()
+		 * callback which also calls free(data) may or may not have been called. Therefore,
+		 * we must guarantee it will have been called by explicitly closing the stream.
+		 * Afterwards, we have no option but to pretend this function was a success. If we
+		 * returned an error, qr_task_send() logic would lead to a double-free because
+		 * on_write() was already called. */
+		nghttp2_submit_rst_stream(h2, NGHTTP2_FLAG_NONE, stream_id, NGHTTP2_INTERNAL_ERROR);
+		return 0;
 	}
 
 	return 0;
@@ -599,6 +610,10 @@ static int http_send_response(nghttp2_session *h2, int32_t stream_id,
 
 /*
  * Send HTTP/2 stream data created from packet's wire buffer.
+ *
+ * If this function returns an error, the on_write() callback isn't (and
+ * musn't be!) called, since such errors are handled in an upper layer - in
+ * qr_task_step() in daemon/worker.
  */
 static int http_write_pkt(nghttp2_session *h2, knot_pkt_t *pkt, int32_t stream_id,
 			  uv_write_t *req, uv_write_cb on_write)