From 372b7377b4350be8c2218fc4e49e16d0f5b33c1a Mon Sep 17 00:00:00 2001
From: Azat Khuzhin <azat@libevent.org>
Date: Tue, 20 Feb 2024 09:30:22 +0100
Subject: [PATCH] Fix leak in evbuffer_add_file() on empty files

Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257
v2: adjust test
---
 buffer.c                | 14 ++++++++++++--
 include/event2/buffer.h |  4 ++--
 test/regress_buffer.c   | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/buffer.c b/buffer.c
index 37c1157f2e..b83c22671e 100644
--- a/buffer.c
+++ b/buffer.c
@@ -3087,6 +3087,7 @@ evbuffer_file_segment_materialize(struct evbuffer_file_segment *seg)
 		return 0; /* already materialized */
 
 #if defined(EVENT__HAVE_MMAP)
+	fprintf(stderr, "%s: EVENT__HAVE_MMAP\n", __func__);
 	if (!(flags & EVBUF_FS_DISABLE_MMAP)) {
 		off_t offset_rounded = 0, offset_leftover = 0;
 		void *mapped;
@@ -3126,15 +3127,18 @@ evbuffer_file_segment_materialize(struct evbuffer_file_segment *seg)
 	}
 #endif
 #ifdef _WIN32
+	fprintf(stderr, "%s: _WIN32\n", __func__);
 	if (!(flags & EVBUF_FS_DISABLE_MMAP)) {
 		intptr_t h = _get_osfhandle(fd);
 		HANDLE m;
 		ev_uint64_t total_size = length+offset;
 		if ((HANDLE)h == INVALID_HANDLE_VALUE)
 			goto err;
+		fprintf(stderr, "%s: before CreateFileMapping\n", __func__);
 		m = CreateFileMapping((HANDLE)h, NULL, PAGE_READONLY,
 		    (total_size >> 32), total_size & 0xfffffffful,
 		    NULL);
+		fprintf(stderr, "%s: after CreateFileMapping\n", __func__);
 		if (m != INVALID_HANDLE_VALUE) { /* Does h leak? */
 			seg->mapping_handle = m;
 			seg->mmap_offset = offset;
@@ -3259,6 +3263,8 @@ evbuffer_add_file_segment(struct evbuffer *buf,
 	struct evbuffer_chain_file_segment *extra;
 	int can_use_sendfile = 0;
 
+	fprintf(stderr, "%s: enter\n", __func__);
+
 	EVBUFFER_LOCK(buf);
 	EVLOCK_LOCK(seg->lock, 0);
 	if (buf->flags & EVBUFFER_FLAG_DRAINS_TO_FD) {
@@ -3266,8 +3272,7 @@ evbuffer_add_file_segment(struct evbuffer *buf,
 	} else {
 		if (evbuffer_file_segment_materialize(seg)<0) {
 			EVLOCK_UNLOCK(seg->lock, 0);
-			EVBUFFER_UNLOCK(buf);
-			return -1;
+			goto err;
 		}
 	}
 	EVLOCK_UNLOCK(seg->lock, 0);
@@ -3308,12 +3313,14 @@ evbuffer_add_file_segment(struct evbuffer *buf,
 			offset_remaining = total_offset % si.dwAllocationGranularity;
 			offset_rounded = total_offset - offset_remaining;
 		}
+		fprintf(stderr, "%s: before MapViewOfFile\n", __func__);
 		data = MapViewOfFile(
 			seg->mapping_handle,
 			FILE_MAP_READ,
 			offset_rounded >> 32,
 			offset_rounded & 0xfffffffful,
 			length + offset_remaining);
+		fprintf(stderr, "%s: after MapViewOfFile\n", __func__);
 		if (data == NULL) {
 			mm_free(chain);
 			goto err;
@@ -3358,7 +3365,10 @@ evbuffer_add_file(struct evbuffer *buf, int fd, ev_off_t offset, ev_off_t length
 	unsigned flags = EVBUF_FS_CLOSE_ON_FREE;
 	int r;
 
+	fprintf(stderr, "%s: enter\n", __func__);
+
 	seg = evbuffer_file_segment_new(fd, offset, length, flags);
+	fprintf(stderr, "%s: seg %p\n", __func__, seg);
 	if (!seg)
 		return -1;
 	r = evbuffer_add_file_segment(buf, seg, 0, length);
diff --git a/include/event2/buffer.h b/include/event2/buffer.h
index 5e225fb9ad..16f0c07827 100644
--- a/include/event2/buffer.h
+++ b/include/event2/buffer.h
@@ -555,8 +555,8 @@ int evbuffer_add_reference_with_offset(struct evbuffer *outbuf, const void *data
   flag is set, it uses those functions.  Otherwise, it tries to use
   mmap (or CreateFileMapping on Windows).
 
-  The function owns the resulting file descriptor and will close it
-  when finished transferring data.
+  The function owns the resulting file descriptor and will close (even in case
+  of error) it when finished transferring data.
 
   The results of using evbuffer_remove() or evbuffer_pullup() on
   evbuffers whose data was added using this function are undefined.
diff --git a/test/regress_buffer.c b/test/regress_buffer.c
index 01c642204e..21fec2d875 100644
--- a/test/regress_buffer.c
+++ b/test/regress_buffer.c
@@ -1107,6 +1107,42 @@ addfile_test_readcb(evutil_socket_t fd, short what, void *arg)
 	}
 }
 
+static void
+test_evbuffer_add_file_leak1(void *ptr)
+{
+	struct basic_test_data *testdata = ptr;
+	struct evbuffer *buf = NULL;
+	char *tmpfilename = NULL;
+	int fd;
+
+	(void)testdata;
+
+	fd = regress_make_tmpfile("", 0, &tmpfilename);
+	/* On Windows, if TMP environment variable is corrupted, we may not be
+	 * able create temporary file, just skip it */
+	if (fd < 0)
+		tt_skip();
+	TT_BLATHER(("Temporary path: %s, fd: %i", tmpfilename, fd));
+
+	/* close fd before usage, so that the fallback with pread() will fail (in
+	 * evbuffer_file_segment_materialize()) */
+	close(fd);
+
+	/* mmap(offset=0, length=0) will fail, this is enough */
+	buf = evbuffer_new();
+	tt_assert(evbuffer_add_file(buf, fd, 0, 0) == -1);
+	evbuffer_validate(buf);
+
+end:
+	if (tmpfilename) {
+		unlink(tmpfilename);
+		free(tmpfilename);
+	}
+	if (buf)
+		evbuffer_free(buf);
+	/* NOTE: file will be closed in evbuffer_add_file() */
+}
+
 static void
 test_evbuffer_add_file(void *ptr)
 {
@@ -2922,6 +2958,7 @@ struct testcase_t evbuffer_testcases[] = {
 	{ "copyout", test_evbuffer_copyout, 0, NULL, NULL},
 	{ "file_segment_add_cleanup_cb", test_evbuffer_file_segment_add_cleanup_cb, 0, NULL, NULL },
 	{ "pullup_with_empty", test_evbuffer_pullup_with_empty, 0, NULL, NULL },
+	{ "add_file_leak1", test_evbuffer_add_file_leak1, 0, NULL, NULL },
 
 #define ADDFILE_TEST(name, parameters)					\
 	{ name, test_evbuffer_add_file, TT_FORK|TT_NEED_BASE,		\