From 8cc19250b324c90f4283bcad488c8bbc756145c4 Mon Sep 17 00:00:00 2001 From: Hoyoung Lee Date: Tue, 22 Jul 2025 17:41:01 +0000 Subject: [PATCH 1/4] t/helper/test-truncate: close file descriptor after truncation Fix a resource leak where the file descriptor was not closed after truncating a file in t/helper/test-truncate.c. Signed-off-by: Hoyoung Lee Signed-off-by: Junio C Hamano --- t/helper/test-truncate.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c index 3931deaec7..2820cc7ed7 100644 --- a/t/helper/test-truncate.c +++ b/t/helper/test-truncate.c @@ -21,5 +21,8 @@ int cmd__truncate(int argc, const char **argv) if (ftruncate(fd, (off_t) sz) < 0) die_errno("failed to truncate file"); + + close(fd); + return 0; } From bc235a68c87d92dc15e2d656a004e9b20042405f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jul 2025 20:00:56 -0400 Subject: [PATCH 2/4] test-delta: handle errors with die() This is a short test helper that does all of its work in the main function. When we encounter an error, we try to clean up memory and descriptors and then jump to an error return, which exits the program. We can get the same effect by just calling die(), which means we do not have to bother with cleaning up. This simplifies the code, and also removes some inconsistencies where a few code paths forgot to clean up descriptors (though in practice it was not a big deal since we were exiting anyway). In addition to die() and die_errno(), we'll also use a few of our usual helpers like xopen() and usage() that make things more ergonomic. This does change the exit code in these cases from 1 to 128, but I don't think it matters (and arguably is better, as we'd already exit 128 for other errors like xmalloc() failure). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/helper/test-delta.c | 55 ++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 37 deletions(-) diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c index 6bc787a474..4495b32b49 100644 --- a/t/helper/test-delta.c +++ b/t/helper/test-delta.c @@ -21,39 +21,26 @@ int cmd__delta(int argc, const char **argv) struct stat st; void *from_buf = NULL, *data_buf = NULL, *out_buf = NULL; unsigned long from_size, data_size, out_size; - int ret = 1; - if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) { - fprintf(stderr, "usage: %s\n", usage_str); - return 1; - } + if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) + usage(usage_str); - fd = open(argv[2], O_RDONLY); - if (fd < 0 || fstat(fd, &st)) { - perror(argv[2]); - return 1; - } + fd = xopen(argv[2], O_RDONLY); + if (fstat(fd, &st) < 0) + die_errno("fstat(%s)", argv[2]); from_size = st.st_size; from_buf = xmalloc(from_size); - if (read_in_full(fd, from_buf, from_size) < 0) { - perror(argv[2]); - close(fd); - goto cleanup; - } + if (read_in_full(fd, from_buf, from_size) < 0) + die_errno("read(%s)", argv[2]); close(fd); - fd = open(argv[3], O_RDONLY); - if (fd < 0 || fstat(fd, &st)) { - perror(argv[3]); - goto cleanup; - } + fd = xopen(argv[3], O_RDONLY); + if (fstat(fd, &st) < 0) + die_errno("fstat(%s)", argv[3]); data_size = st.st_size; data_buf = xmalloc(data_size); - if (read_in_full(fd, data_buf, data_size) < 0) { - perror(argv[3]); - close(fd); - goto cleanup; - } + if (read_in_full(fd, data_buf, data_size) < 0) + die_errno("read(%s)", argv[3]); close(fd); if (argv[1][1] == 'd') @@ -64,22 +51,16 @@ int cmd__delta(int argc, const char **argv) out_buf = patch_delta(from_buf, from_size, data_buf, data_size, &out_size); - if (!out_buf) { - fprintf(stderr, "delta operation failed (returned NULL)\n"); - goto cleanup; - } + if (!out_buf) + die("delta operation failed (returned NULL)"); - fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666); - if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) { - perror(argv[4]); - goto cleanup; - } + fd = xopen(argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666); + if (write_in_full(fd, out_buf, out_size) < 0) + die_errno("write(%s)", argv[4]); - ret = 0; -cleanup: free(from_buf); free(data_buf); free(out_buf); - return ret; + return 0; } From 760dd804bb40b613396d25a0905db7fe4a52b00f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jul 2025 20:02:11 -0400 Subject: [PATCH 3/4] test-delta: use strbufs to hold input files We want to read the whole contents of two files into memory. If we switch from raw ptr/len pairs to strbufs, we can use strbuf_read_file() to shorten the code. This incidentally fixes two small bugs: 1. We stat() the files and allocate our buffers based on st.st_size. But that is an off_t which may be larger than the size_t we'd use to allocate. We should use xsize_t() to do a checked conversion. Otherwise integer truncation (on a file >4GB) could cause us to under-allocate (though in practice this does not result in a buffer overflow because the same truncation happens when read_in_full() also takes a size_t). 2. We get the size from st.st_size, and then try to read_in_full() that many bytes. But it may return fewer bytes than expected (if the file changed racily and we get an early EOF), leading us to read uninitialized bytes in the allocated buffer. We don't notice because we only check the value for error, not that we got the expected number of bytes. The strbuf code doesn't run into this, because it just reads to EOF, expanding the buffer dynamically as necessary. Neither bug is a big deal for a test helper, but fixing them is a nice bonus on top of simplifying the code. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/helper/test-delta.c | 40 ++++++++++++++-------------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c index 4495b32b49..7945793078 100644 --- a/t/helper/test-delta.c +++ b/t/helper/test-delta.c @@ -11,6 +11,7 @@ #include "test-tool.h" #include "git-compat-util.h" #include "delta.h" +#include "strbuf.h" static const char usage_str[] = "test-tool delta (-d|-p) "; @@ -18,38 +19,25 @@ static const char usage_str[] = int cmd__delta(int argc, const char **argv) { int fd; - struct stat st; - void *from_buf = NULL, *data_buf = NULL, *out_buf = NULL; - unsigned long from_size, data_size, out_size; + struct strbuf from = STRBUF_INIT, data = STRBUF_INIT; + char *out_buf; + unsigned long out_size; if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) usage(usage_str); - fd = xopen(argv[2], O_RDONLY); - if (fstat(fd, &st) < 0) - die_errno("fstat(%s)", argv[2]); - from_size = st.st_size; - from_buf = xmalloc(from_size); - if (read_in_full(fd, from_buf, from_size) < 0) - die_errno("read(%s)", argv[2]); - close(fd); - - fd = xopen(argv[3], O_RDONLY); - if (fstat(fd, &st) < 0) - die_errno("fstat(%s)", argv[3]); - data_size = st.st_size; - data_buf = xmalloc(data_size); - if (read_in_full(fd, data_buf, data_size) < 0) - die_errno("read(%s)", argv[3]); - close(fd); + if (strbuf_read_file(&from, argv[2], 0) < 0) + die_errno("unable to read '%s'", argv[2]); + if (strbuf_read_file(&data, argv[3], 0) < 0) + die_errno("unable to read '%s'", argv[3]); if (argv[1][1] == 'd') - out_buf = diff_delta(from_buf, from_size, - data_buf, data_size, + out_buf = diff_delta(from.buf, from.len, + data.buf, data.len, &out_size, 0); else - out_buf = patch_delta(from_buf, from_size, - data_buf, data_size, + out_buf = patch_delta(from.buf, from.len, + data.buf, data.len, &out_size); if (!out_buf) die("delta operation failed (returned NULL)"); @@ -58,8 +46,8 @@ int cmd__delta(int argc, const char **argv) if (write_in_full(fd, out_buf, out_size) < 0) die_errno("write(%s)", argv[4]); - free(from_buf); - free(data_buf); + strbuf_release(&from); + strbuf_release(&data); free(out_buf); return 0; From 0f1b33815b553dd457f8e38e3768b73cf9227082 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 23 Jul 2025 20:03:08 -0400 Subject: [PATCH 4/4] test-delta: close output descriptor after use After we write to the output file, the program exits. This naturally closes the descriptor. But we should do an explicit close for two reasons: 1. It's possible to hit an error on close(), which we should detect and report via our exit code. 2. Leaking descriptors is a bad practice in general. Even if it isn't meaningful here, it sets a bad example. It is tempting to write: if (write_in_full(fd, ...) < 0 || close(fd) < 0) die_errno(...); But that pattern contains a subtle problem that has resulted in descriptor leaks before. If write_in_full() fails, we'll short-circuit and never call close(), leaking the descriptor. That's not a problem here, since our error path dies instead of returning up the stack. But since we're trying to set a good example, let's write it out as two separate conditions. As a bonus, that lets us produce a slightly more specific error message. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/helper/test-delta.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c index 7945793078..52ea00c937 100644 --- a/t/helper/test-delta.c +++ b/t/helper/test-delta.c @@ -45,6 +45,8 @@ int cmd__delta(int argc, const char **argv) fd = xopen(argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666); if (write_in_full(fd, out_buf, out_size) < 0) die_errno("write(%s)", argv[4]); + if (close(fd) < 0) + die_errno("close(%s)", argv[4]); strbuf_release(&from); strbuf_release(&data);