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 f72465f65e1313e187cd763815077d0a416fa8ee..a743e7f09a141032ce1147017dc51fcddb203894 100644 (file)
--- 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)