Merge branch 'evbuffer_add_file-2.0-fixes' into patches-2.0

Bunch of fixes for evbuffer_add_file().

* evbuffer_add_file-2.0-fixes:
  Cover evbuffer_add_file() with offset
  evbuffer_add_file: do not use evbuffer_remove(), instead calc offset for mmap()
  evbuffer_add_file: munmap() correct size on mmap() failure
  evbuffer_add_file: fix endless loop when file does not have such amount of data

Fixes: #306

2 files changed, 90 insertions, 44 deletions

diff --git a/buffer.c b/buffer.c
index 86793d61..da44eef8 100644
--- a/buffer.c
+++ b/buffer.c
@@ -2769,6 +2769,20 @@ done:
 	return result;
 }
 
+#ifdef _EVENT_HAVE_MMAP
+static long
+get_page_size(void)
+{
+#ifdef SC_PAGE_SIZE
+	return sysconf(SC_PAGE_SIZE);
+#elif defined(_SC_PAGE_SIZE)
+	return sysconf(_SC_PAGE_SIZE);
+#else
+	return 1;
+#endif
+}
+#endif
+
 /* TODO(niels): maybe we don't want to own the fd, however, in that
  * case, we should dup it - dup is cheap.  Perhaps, we should use a
  * callback instead?
@@ -2829,7 +2843,20 @@ evbuffer_add_file(struct evbuffer *outbuf, int fd,
 #endif
 #if defined(_EVENT_HAVE_MMAP)
 	if (use_mmap) {
-		void *mapped = mmap(NULL, length + offset, PROT_READ,
+		off_t offset_rounded = 0, offset_leftover = 0;
+		void *mapped;
+		if (offset) {
+			/* mmap implementations don't generally like us
+			 * to have an offset that isn't a round  */
+			long page_size = get_page_size();
+			if (page_size == -1) {
+				event_warn("%s: cannot detect PAGE_SIZE", __func__);
+				return (-1);
+			}
+			offset_leftover = offset % page_size;
+			offset_rounded = offset - offset_leftover;
+		}
+		mapped = mmap(NULL, length + offset_leftover, PROT_READ,
 #ifdef MAP_NOCACHE
 		    MAP_NOCACHE |
 #endif
@@ -2837,29 +2864,24 @@ evbuffer_add_file(struct evbuffer *outbuf, int fd,
 		    MAP_FILE |
 #endif
 		    MAP_PRIVATE,
-		    fd, 0);
-		/* some mmap implementations require offset to be a multiple of
-		 * the page size.  most users of this api, are likely to use 0
-		 * so mapping everything is not likely to be a problem.
-		 * TODO(niels): determine page size and round offset to that
-		 * page size to avoid mapping too much memory.
-		 */
+		    fd, offset_rounded);
 		if (mapped == MAP_FAILED) {
 			event_warn("%s: mmap(%d, %d, %zu) failed",
-			    __func__, fd, 0, (size_t)(offset + length));
+			    __func__, fd, 0, (size_t)(length + offset));
 			return (-1);
 		}
 		chain = evbuffer_chain_new(sizeof(struct evbuffer_chain_fd));
 		if (chain == NULL) {
 			event_warn("%s: out of memory", __func__);
-			munmap(mapped, length);
+			munmap(mapped, length + offset_leftover);
 			return (-1);
 		}
 
 		chain->flags |= EVBUFFER_MMAP | EVBUFFER_IMMUTABLE;
 		chain->buffer = mapped;
-		chain->buffer_len = length + offset;
-		chain->off = length + offset;
+		chain->buffer_len = length + offset_leftover;
+		chain->off = length;
+		chain->misalign = offset_leftover;
 
 		info = EVBUFFER_CHAIN_EXTRA(struct evbuffer_chain_fd, chain);
 		info->fd = fd;
@@ -2871,11 +2893,7 @@ evbuffer_add_file(struct evbuffer *outbuf, int fd,
 			ok = 0;
 		} else {
 			outbuf->n_add_for_cb += length;
-
 			evbuffer_chain_insert(outbuf, chain);
-
-			/* we need to subtract whatever we don't need */
-			evbuffer_drain(outbuf, offset);
 		}
 	} else
 #endif
@@ -2901,13 +2919,14 @@ evbuffer_add_file(struct evbuffer *outbuf, int fd,
 		while (length) {
 			ev_ssize_t to_read = length > EV_SSIZE_MAX ? EV_SSIZE_MAX : (ev_ssize_t)length;
 			read = evbuffer_readfile(tmp, fd, to_read);
-			if (read == -1) {
-				evbuffer_free(tmp);
-				return (-1);
-			}
-
+			if (read <= 0)
+				break;
 			length -= read;
 		}
+		if (length) {
+			evbuffer_free(tmp);
+			return (-1);
+		}
 
 		EVBUFFER_LOCK(outbuf);
 		if (outbuf->freeze_end) {
diff --git a/test/regress_buffer.c b/test/regress_buffer.c
index b6b4bb93..dafebe15 100644
--- a/test/regress_buffer.c
+++ b/test/regress_buffer.c
@@ -713,30 +713,46 @@ test_evbuffer_add_file(void *ptr)
 {
 	const char *impl = ptr;
 	struct evbuffer *src = evbuffer_new();
-	const char *data = "this is what we add as file system data.";
-	size_t datalen;
+	char *data = strdup("this is what we add as file system data.");
+	size_t datalen = strlen(data), toread = datalen, bufread = 0;
 	const char *compare;
 	int fd = -1;
 	evutil_socket_t pair[2] = {-1, -1};
 	int r=0, n_written=0;
-
-	/* Add a test for a big file. XXXX */
+	ev_off_t offset = 0;
 
 	tt_assert(impl);
-	if (!strcmp(impl, "sendfile")) {
+	if (strstr(impl, "_sendfile")) {
 		if (!_evbuffer_testing_use_sendfile())
 			tt_skip();
 		TT_BLATHER(("Using sendfile-based implementaion"));
-	} else if (!strcmp(impl, "mmap")) {
+	} else if (strstr(impl, "_mmap")) {
 		if (!_evbuffer_testing_use_mmap())
 			tt_skip();
 		TT_BLATHER(("Using mmap-based implementaion"));
-	} else if (!strcmp(impl, "linear")) {
+	} else if (strstr(impl, "_linear")) {
 		if (!_evbuffer_testing_use_linear_file_access())
 			tt_skip();
 		TT_BLATHER(("Using read-based implementaion"));
-	} else {
-		TT_DIE(("Didn't recognize the implementation"));
+	}
+
+	if (strstr(impl, "_big")) {
+		toread = datalen = 1 << 16;
+		data = realloc(data, datalen);
+		for (int i = 0; i < 1 << 16; ++i) {
+			data[i] = (char)_evutil_weakrand() % sizeof(char);
+		}
+		TT_BLATHER(("Big file"));
+	}
+	if (strstr(impl, "_offset")) {
+		offset = datalen / 4;
+		toread = datalen - offset;
+		TT_BLATHER(("Read from offset"));
+	}
+	if (strstr(impl, "_noalign")) {
+		++offset;
+		--toread;
+		TT_BLATHER(("Read from non aligned offset"));
 	}
 
 	/* Say that it drains to a fd so that we can use sendfile. */
@@ -752,13 +768,10 @@ test_evbuffer_add_file(void *ptr)
 		tt_abort_msg("socketpair failed");
 #endif
 
-	datalen = strlen(data);
 	fd = regress_make_tmpfile(data, datalen);
-
 	tt_assert(fd != -1);
 
-	tt_assert(evbuffer_add_file(src, fd, 0, datalen) != -1);
-
+	tt_assert(evbuffer_add_file(src, fd, offset, toread) != -1);
 	evbuffer_validate(src);
 
 	while (evbuffer_get_length(src) &&
@@ -767,14 +780,20 @@ test_evbuffer_add_file(void *ptr)
 		n_written += r;
 	}
 	tt_int_op(r, !=, -1);
-	tt_int_op(n_written, ==, datalen);
+	tt_int_op(n_written, ==, toread);
 
 	evbuffer_validate(src);
-	tt_int_op(evbuffer_read(src, pair[1], (int)strlen(data)), ==, datalen);
+
+	for (int n = toread, bufread = toread;
+		 n > 0;
+		 n = evbuffer_read(src, pair[1], (int)bufread), bufread -= n) {
+	}
+	tt_int_op(bufread, ==, 0);
+
 	evbuffer_validate(src);
-	compare = (char *)evbuffer_pullup(src, datalen);
+	compare = (char *)evbuffer_pullup(src, toread);
 	tt_assert(compare != NULL);
-	if (memcmp(compare, data, datalen))
+	if (memcmp(compare, data + offset, toread))
 		tt_abort_msg("Data from add_file differs.");
 
 	evbuffer_validate(src);
@@ -784,6 +803,7 @@ test_evbuffer_add_file(void *ptr)
 	if (pair[1] >= 0)
 		evutil_closesocket(pair[1]);
 	evbuffer_free(src);
+	free(data);
 }
 
 #ifndef _EVENT_DISABLE_MM_REPLACEMENT
@@ -1753,12 +1773,19 @@ struct testcase_t evbuffer_testcases[] = {
 	{ "freeze_start", test_evbuffer_freeze, 0, &nil_setup, (void*)"start" },
 	{ "freeze_end", test_evbuffer_freeze, 0, &nil_setup, (void*)"end" },
 	/* TODO: need a temp file implementation for Windows */
-	{ "add_file_sendfile", test_evbuffer_add_file, TT_FORK, &nil_setup,
-	  (void*)"sendfile" },
-	{ "add_file_mmap", test_evbuffer_add_file, TT_FORK, &nil_setup,
-	  (void*)"mmap" },
-	{ "add_file_linear", test_evbuffer_add_file, TT_FORK, &nil_setup,
-	  (void*)"linear" },
+
+#define ADD_FILE_TEST(name) \
+	{ name, test_evbuffer_add_file, TT_FORK, &nil_setup, (void*)name },
+#define ADD_FILE_TEST_GROUP(name) \
+	ADD_FILE_TEST(name) \
+	ADD_FILE_TEST(name "_offset") \
+	ADD_FILE_TEST(name "_big") \
+	ADD_FILE_TEST(name "_big_offset") \
+	ADD_FILE_TEST(name "_big_offset_noalign") \
+
+	ADD_FILE_TEST_GROUP("add_file_sendfile")
+	ADD_FILE_TEST_GROUP("add_file_mmap")
+	ADD_FILE_TEST_GROUP("add_file_linear")
 
 	END_OF_TESTCASES
 };