{"thread":{"id":"63822","subject":"[PATCH v3 0/2] fix resource leaks in test helpers","startedAt":"2025-07-22T17:41:15Z","lastAt":"2025-07-24T00:03:09Z","messageCount":18,"participants":["Hoyoung Lee","Junio C Hamano","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"522477","messageId":"20250722174102.1876197-1-lhywkd22@gmail.com","threadId":"63822","inReplyTo":null,"subject":"[PATCH v3 0/2] fix resource leaks in test helpers","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-22T17:41:00Z","receivedAt":"2025-07-22T17:41:15Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"Thanks for the detailed and thoughtful reviews. Your comments helped me better understand how resource cleanup should be handled, especially when dealing with early program termination.\n\nI especially appreciate your in-depth explanation regarding the difference between true resource leaks and stack-unwinding cases. The example you gave was very insightful, and it clarified how automated tools and reviewers should assess whether a resource is truly leaked. Thank you for taking the time to provide such a clear and instructive explanation.\n\nHoyoung Lee (2):\n t/helper/test-truncate: close file descriptor after truncation\n t/helper/test-delta: fix possible resource leak and ensure safe\n cleanup\n\n t/helper/test-delta.c | 9 +++++----\n t/helper/test-truncate.c | 3 +++\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"522478","messageId":"20250722174102.1876197-2-lhywkd22@gmail.com","threadId":"63822","inReplyTo":"20250722174102.1876197-1-lhywkd22@gmail.com","subject":"[PATCH v3 1/2] t/helper/test-truncate: close file descriptor after truncation","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-22T17:41:01Z","receivedAt":"2025-07-22T17:41:18Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"Fix a resource leak where the file descriptor was not closed after\ntruncating a file in t/helper/test-truncate.c.\n\nSigned-off-by: Hoyoung Lee \n---\n t/helper/test-truncate.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c\nindex 3931deaec7..104bc36cc0 100644\n--- a/t/helper/test-truncate.c\n+++ b/t/helper/test-truncate.c\n@@ -21,5 +21,8 @@ int cmd__truncate(int argc, const char **argv)\n \n \tif (ftruncate(fd, (off_t) sz) < 0)\n \t\tdie_errno(\"failed to truncate file\");\n+\t\n+\tclose(fd);\n+\n \treturn 0;\n }\n-- \n2.34.1\n\n"},{"id":"522479","messageId":"20250722174102.1876197-3-lhywkd22@gmail.com","threadId":"63822","inReplyTo":"20250722174102.1876197-1-lhywkd22@gmail.com","subject":"[PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-22T17:41:02Z","receivedAt":"2025-07-22T17:41:19Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"Initialize `fd` to -1 and unify all `open()`-related `close()` calls\nunder a single cleanup label. This prevents undefined behavior when\n`fd` is used without initialization in error paths.\n\nThe cleanup logic now safely avoids calling `close()` on invalid\ndescriptors and ensures consistent resource management.\n\nSigned-off-by: Hoyoung Lee \n---\n t/helper/test-delta.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 6bc787a474..f5811e96ad 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -17,7 +17,7 @@ static const char usage_str[] =\n \n int cmd__delta(int argc, const char **argv)\n {\n-\tint fd;\n+\tint fd = -1;\n \tstruct stat st;\n \tvoid *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;\n \tunsigned long from_size, data_size, out_size;\n@@ -31,13 +31,12 @@ int cmd__delta(int argc, const char **argv)\n \tfd = open(argv[2], O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st)) {\n \t\tperror(argv[2]);\n-\t\treturn 1;\n+\t\tgoto cleanup;\n \t}\n \tfrom_size = st.st_size;\n \tfrom_buf = xmalloc(from_size);\n \tif (read_in_full(fd, from_buf, from_size) < 0) {\n \t\tperror(argv[2]);\n-\t\tclose(fd);\n \t\tgoto cleanup;\n \t}\n \tclose(fd);\n@@ -51,7 +50,6 @@ int cmd__delta(int argc, const char **argv)\n \tdata_buf = xmalloc(data_size);\n \tif (read_in_full(fd, data_buf, data_size) < 0) {\n \t\tperror(argv[3]);\n-\t\tclose(fd);\n \t\tgoto cleanup;\n \t}\n \tclose(fd);\n@@ -81,5 +79,8 @@ int cmd__delta(int argc, const char **argv)\n \tfree(data_buf);\n \tfree(out_buf);\n \n+\tif (fd >= 0)\n+\t\tclose(fd);\n+\n \treturn ret;\n }\n-- \n2.34.1\n\n"},{"id":"522501","messageId":"xmqqqzy7rj6k.fsf@gitster.g","threadId":"63822","inReplyTo":"20250722174102.1876197-2-lhywkd22@gmail.com","subject":"Re: [PATCH v3 1/2] t/helper/test-truncate: close file descriptor after truncation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-22T21:48:19Z","receivedAt":"2025-07-22T21:48:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hoyoung Lee writes:\n\n> Fix a resource leak where the file descriptor was not closed after\n> truncating a file in t/helper/test-truncate.c.\n>\n> Signed-off-by: Hoyoung Lee \n> ---\n> t/helper/test-truncate.c | 3 +++\n> 1 file changed, 3 insertions(+)\n\nWhile it is not wrong per-se, it is not like a function that can\npotentially be called number of times returns without closing it.\nNobody other than the main() function would be calling this\nfunction, so the only thing that is done after this function returns\nto its caller is for the caller to return leading to exit from the\nprocess at which point the leftover file descriptor would be\nclosed.\n\nSo I am a bit curious what triggered you to send in these changes.\nAre there some automated resource leak checker, without fixes like\nthese whose output would be noisy with complaints about these \"known\nleaking, no ill effects in practice, yet gets flagged by checkers\"\ncode to be useful, or something?\n\nWill queue; thanks.\n\n> diff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c\n> index 3931deaec7..104bc36cc0 100644\n> --- a/t/helper/test-truncate.c\n> +++ b/t/helper/test-truncate.c\n> @@ -21,5 +21,8 @@ int cmd__truncate(int argc, const char **argv)\n> \n> \tif (ftruncate(fd, (off_t) sz) < 0)\n> \t\tdie_errno(\"failed to truncate file\");\n> +\t\n> +\tclose(fd);\n> +\n> \treturn 0;\n> }\n"},{"id":"522529","messageId":"CAPig+cR-r=CeEaSTeWsX00MLCSRJUUVXMUWS6Ui-HQcR_qMGJA@mail.gmail.com","threadId":"63822","inReplyTo":"20250722174102.1876197-3-lhywkd22@gmail.com","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-07-23T07:28:05Z","receivedAt":"2025-07-23T07:28:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 22, 2025 at 1:41 PM Hoyoung Lee wrote:\n> Initialize `fd` to -1 and unify all `open()`-related `close()` calls\n> under a single cleanup label. This prevents undefined behavior when\n> `fd` is used without initialization in error paths.\n\nIt's not clear what this means. As far as I can tell, the original\ncode never used an uninitialized `fd` in error paths.\n\n> The cleanup logic now safely avoids calling `close()` on invalid\n> descriptors and ensures consistent resource management.\n\nAgain, it's not clear what this means. Although your previous version\nof this patch did add a call to close() with an invalid descriptor,\nthe original code did not do so. So, the above statement seems to be\nmisleading.\n\nThose issues aside, the patch itself has problems, some minor, such as\nmaking the code a bit confusing or misleading, and some major, such as\ncalling close() on an already closed descriptor.\n\nUnfortunately, there are enough important context lines missing from\nthe patch itself that, instead of directly reviewing the patch\ndirectly, I'm going to review the code following the application of\nyour patch...\n\n> int fd = -1;\n\nThis new initialization (-1) is useless because...\n\n> if (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\"))) {\n> fprintf(stderr, \"usage: %s\\n\", usage_str);\n> return 1;\n> }\n>\n> fd = open(argv[2], O_RDONLY);\n\n...the very first time `fd` is mentioned (aside from the declaration)\nis here where it is unconditionally assigned a value. Thus, the -1\ninitialization is wasted (and potentially confusing for readers).\n\n> if (fd < 0 || fstat(fd, &st)) {\n> perror(argv[2]);\n> goto cleanup;\n> }\n\nOkay, no problem here. The `if (fd >= 0) close(fd)` you added to the\n\"cleanup\" action handles both the cases here when `fd` might be\nnegative after the open() call or a valid descriptor.\n\n> from_size = st.st_size;\n> from_buf = xmalloc(from_size);\n> if (read_in_full(fd, from_buf, from_size) < 0) {\n> perror(argv[2]);\n> goto cleanup;\n> }\n> close(fd);\n\nHere `fd` is closed manually which is good because...\n\n> fd = open(argv[3], O_RDONLY);\n\n...this code immediately assigns it a new value.\n\n> if (fd < 0 || fstat(fd, &st)) {\n> perror(argv[3]);\n> goto cleanup;\n> }\n\nOkay for the same reason mentioned above.\n\n> data_size = st.st_size;\n> data_buf = xmalloc(data_size);\n> if (read_in_full(fd, data_buf, data_size) < 0) {\n> perror(argv[3]);\n> goto cleanup;\n> }\n> close(fd);\n\nThe descriptor is closed manually (again) because a subsequent open()\ncall is going to reuse the variable. However...\n\n> if (argv[1][1] == 'd')\n> out_buf = diff_delta(from_buf, from_size,\n> data_buf, data_size,\n> &out_size, 0);\n> else\n> out_buf = patch_delta(from_buf, from_size,\n> data_buf, data_size,\n> &out_size);\n> if (!out_buf) {\n> fprintf(stderr, \"delta operation failed (returned NULL)\\n\");\n> goto cleanup;\n> }\n\n...although `fd` was closed, it still holds the previously-open\nnon-negative file descriptor, which means that this `goto cleanup`...\n\n> fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n> if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {\n> perror(argv[4]);\n> goto cleanup;\n> }\n>\n> ret = 0;\n> cleanup:\n> free(from_buf);\n> free(data_buf);\n> free(out_buf);\n>\n> if (fd >= 0)\n> close(fd);\n\n...will arrive here and the condition will evaluate to \"true\",\nresulting in the already-closed descriptor being closed again.\n\n> return ret;\n"},{"id":"522531","messageId":"20250723075513.GA570540@coredump.intra.peff.net","threadId":"63822","inReplyTo":"CAPig+cR-r=CeEaSTeWsX00MLCSRJUUVXMUWS6Ui-HQcR_qMGJA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-23T07:55:13Z","receivedAt":"2025-07-23T07:55:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 23, 2025 at 03:28:05AM -0400, Eric Sunshine wrote:\n\n> > close(fd);\n> \n> The descriptor is closed manually (again) because a subsequent open()\n> call is going to reuse the variable. However...\n> \n> > if (argv[1][1] == 'd')\n> > out_buf = diff_delta(from_buf, from_size,\n> > data_buf, data_size,\n> > &out_size, 0);\n> > else\n> > out_buf = patch_delta(from_buf, from_size,\n> > data_buf, data_size,\n> > &out_size);\n> > if (!out_buf) {\n> > fprintf(stderr, \"delta operation failed (returned NULL)\\n\");\n> > goto cleanup;\n> > }\n> \n> ...although `fd` was closed, it still holds the previously-open\n> non-negative file descriptor, which means that this `goto cleanup`...\n\nOof, good catch. This iteration of the patch was based on my suggestion,\nbut I didn't notice the jump to cleanup between that close/open pair.\n\nI wonder if it would be more clear written as:\n\n int from_fd = -1;\n int data_fd = -1;\n int out_fd = -1;\n\n ...\n from_fd = open(...);\n if (...errors...)\n\tgoto cleanup;\n close(from_fd);\n from_fd = -1;\n\n [...same for other fds...]\n\ncleanup:\n\n if (from_fd >= 0)\n\tclose(from_fd);\n [etc]\n\nYou could even drop that first close() in the happy path entirely, if\nyou don't mind holding all three descriptors open at once.\n\nI dunno. We are reaching diminishing returns spending brainpower on a\nfunction that is meant to be somewhat quick-and-dirty.\n\n-Peff\n"},{"id":"522533","messageId":"20250723080639.GA570920@coredump.intra.peff.net","threadId":"63822","inReplyTo":"20250723075513.GA570540@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-23T08:06:39Z","receivedAt":"2025-07-23T08:06:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 23, 2025 at 03:55:13AM -0400, Jeff King wrote:\n\n> I dunno. We are reaching diminishing returns spending brainpower on a\n> function that is meant to be somewhat quick-and-dirty.\n\nOK, I clearly could not resist spending more brainpower on it. If we are\ndoing quick-and-dirty, why not just die()? The end result is the same,\nbut per my argument in the earlier iteration of the series, that means\nwe do not have to worry about cleaning up at all.\n\nLike:\n\n t/helper/test-delta.c | 49 ++++++++++++-----------------------\n 1 file changed, 16 insertions(+), 33 deletions(-)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 6bc787a474..769b68839d 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -21,39 +21,28 @@ int cmd__delta(int argc, const char **argv)\n \tstruct stat st;\n \tvoid *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;\n \tunsigned long from_size, data_size, out_size;\n-\tint ret = 1;\n \n \tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\"))) {\n \t\tfprintf(stderr, \"usage: %s\\n\", usage_str);\n \t\treturn 1;\n \t}\n \n-\tfd = open(argv[2], O_RDONLY);\n-\tif (fd < 0 || fstat(fd, &st)) {\n-\t\tperror(argv[2]);\n-\t\treturn 1;\n-\t}\n+\tfd = xopen(argv[2], O_RDONLY);\n+\tif (fstat(fd, &st) < 0)\n+\t\tdie_errno(\"fstat(%s)\", argv[2]);\n \tfrom_size = st.st_size;\n \tfrom_buf = xmalloc(from_size);\n-\tif (read_in_full(fd, from_buf, from_size) < 0) {\n-\t\tperror(argv[2]);\n-\t\tclose(fd);\n-\t\tgoto cleanup;\n-\t}\n+\tif (read_in_full(fd, from_buf, from_size) < 0)\n+\t\tdie_errno(\"read(%s)\", argv[2]);\n \tclose(fd);\n \n-\tfd = open(argv[3], O_RDONLY);\n-\tif (fd < 0 || fstat(fd, &st)) {\n-\t\tperror(argv[3]);\n-\t\tgoto cleanup;\n-\t}\n+\tfd = xopen(argv[3], O_RDONLY);\n+\tif (fstat(fd, &st) < 0)\n+\t\tdie_errno(\"fstat(%s)\", argv[3]);\n \tdata_size = st.st_size;\n \tdata_buf = xmalloc(data_size);\n-\tif (read_in_full(fd, data_buf, data_size) < 0) {\n-\t\tperror(argv[3]);\n-\t\tclose(fd);\n-\t\tgoto cleanup;\n-\t}\n+\tif (read_in_full(fd, data_buf, data_size) < 0)\n+\t\tdie_errno(\"read(%s)\", argv[3]);\n \tclose(fd);\n \n \tif (argv[1][1] == 'd')\n@@ -64,22 +53,16 @@ int cmd__delta(int argc, const char **argv)\n \t\tout_buf = patch_delta(from_buf, from_size,\n \t\t\t\t data_buf, data_size,\n \t\t\t\t &out_size);\n-\tif (!out_buf) {\n-\t\tfprintf(stderr, \"delta operation failed (returned NULL)\\n\");\n-\t\tgoto cleanup;\n-\t}\n+\tif (!out_buf)\n+\t\tdie(\"delta operation failed (returned NULL)\");\n \n-\tfd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n-\tif (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {\n-\t\tperror(argv[4]);\n-\t\tgoto cleanup;\n-\t}\n+\tfd = xopen(argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n+\tif (write_in_full(fd, out_buf, out_size) < 0)\n+\t\tdie_errno(\"write(%s)\", argv[4]);\n \n-\tret = 0;\n-cleanup:\n \tfree(from_buf);\n \tfree(data_buf);\n \tfree(out_buf);\n \n-\treturn ret;\n+\treturn 0;\n }\n\n\nI'd guess that one could probably go even further by replacing the bare\npointers with strbufs. And then you could use strbuf_read_file().\n\nIncidentally that would also fix two minor bugs I noticed:\n\n - passing st.st_size directly to xmalloc() is wrong, because of\n truncation from off_t to size_t. This should use the xsize_t helper.\n This is even a potential security vulnerability, but probably not\n important in a test helper.\n\n - likewise read_in_full() might return a non-negative value smaller\n than the requested size (if the file racily changes and we get an\n early EOF). But we only check whether we got a negative error value.\n So we may read fewer bytes than expected and feed uninitialized\n garbage to the delta code.\n\n-Peff\n"},{"id":"522534","messageId":"CAPig+cQ2p3muA0-8O_u-rOW0uVGQ_ZE7hcz-7qq8M1U+PdvtpQ@mail.gmail.com","threadId":"63822","inReplyTo":"20250723075513.GA570540@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-07-23T08:11:26Z","receivedAt":"2025-07-23T08:11:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 23, 2025 at 3:55 AM Jeff King wrote:\n> On Wed, Jul 23, 2025 at 03:28:05AM -0400, Eric Sunshine wrote:\n> > The descriptor is closed manually (again) because a subsequent open()\n> > call is going to reuse the variable. However...\n> > ...although `fd` was closed, it still holds the previously-open\n> > non-negative file descriptor, which means that this `goto cleanup`...\n>\n> Oof, good catch. This iteration of the patch was based on my suggestion,\n> but I didn't notice the jump to cleanup between that close/open pair.\n>\n> I dunno. We are reaching diminishing returns spending brainpower on a\n> function that is meant to be somewhat quick-and-dirty.\n\nAside from preferring that the patch not make the code worse or more\nconfusing, I don't have a strong opinion. Superficially, the existing\ncode presents itself as being careful by cleaning up after itself, but\nas Hoyoung Lee discovered, there are holes in the cleanup\nimplementation. So, I do like that the intention of the patch is to\nplug those holes, but we don't necessarily need to be particularly\nclever about it. For such simple test-related code, even a pure\nbrute-force fix should be acceptable.\n\nFor completeness, I'll mention that I even had the thought that\nanother \"fix\" would be to tear out all the cleanup code entirely since\nwe _know_ that this function will be exiting immediately and the OS\nwill clean up any dangling resources.\n"},{"id":"522535","messageId":"CAPig+cQAcEiM7aPQZXYX03Ok_qZktWpAgPPS9DUWu3UPVrxx7Q@mail.gmail.com","threadId":"63822","inReplyTo":"20250723080639.GA570920@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-07-23T08:17:01Z","receivedAt":"2025-07-23T08:17:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 23, 2025 at 4:06 AM Jeff King wrote:\n> On Wed, Jul 23, 2025 at 03:55:13AM -0400, Jeff King wrote:\n> > I dunno. We are reaching diminishing returns spending brainpower on a\n> > function that is meant to be somewhat quick-and-dirty.\n>\n> OK, I clearly could not resist spending more brainpower on it. If we are\n> doing quick-and-dirty, why not just die()? The end result is the same,\n> but per my argument in the earlier iteration of the series, that means\n> we do not have to worry about cleaning up at all.\n\nYes, die() seems sensible here. It's nice and tidy and makes the code\neasier to reason about.\n\n> Incidentally that would also fix two minor bugs I noticed:\n>\n> - passing st.st_size directly to xmalloc() is wrong, because of\n> truncation from off_t to size_t. This should use the xsize_t helper.\n> This is even a potential security vulnerability, but probably not\n> important in a test helper.\n>\n> - likewise read_in_full() might return a non-negative value smaller\n> than the requested size (if the file racily changes and we get an\n> early EOF). But we only check whether we got a negative error value.\n> So we may read fewer bytes than expected and feed uninitialized\n> garbage to the delta code.\n\nCan of worms opened.\n"},{"id":"522536","messageId":"20250723084624.GA580010@coredump.intra.peff.net","threadId":"63822","inReplyTo":"CAPig+cQ2p3muA0-8O_u-rOW0uVGQ_ZE7hcz-7qq8M1U+PdvtpQ@mail.gmail.com","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-23T08:46:24Z","receivedAt":"2025-07-23T08:46:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 23, 2025 at 04:11:26AM -0400, Eric Sunshine wrote:\n\n> For completeness, I'll mention that I even had the thought that\n> another \"fix\" would be to tear out all the cleanup code entirely since\n> we _know_ that this function will be exiting immediately and the OS\n> will clean up any dangling resources.\n\nThe reason we have the \"cleanup\" label at all is because of the memory\nleaks. And there the issue is that we build the test helpers with the\nsame compiler settings as the rest of the code, so SANITIZE=leak will\ncomplain. So I think that is a non-starter.\n\nBut if you just meant leaking descriptors, sure, I don't think any tools\ncomplain about that. ;)\n\n-Peff\n"},{"id":"522577","messageId":"CAPig+cSzYPaBGZ_muhxwcNy4xWaMF5Uc0+SN7ygnS4wNaGxEKQ@mail.gmail.com","threadId":"63822","inReplyTo":"20250723084624.GA580010@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-07-23T16:37:16Z","receivedAt":"2025-07-23T16:37:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 23, 2025 at 4:46 AM Jeff King wrote:\n> On Wed, Jul 23, 2025 at 04:11:26AM -0400, Eric Sunshine wrote:\n> > For completeness, I'll mention that I even had the thought that\n> > another \"fix\" would be to tear out all the cleanup code entirely since\n> > we _know_ that this function will be exiting immediately and the OS\n> > will clean up any dangling resources.\n>\n> The reason we have the \"cleanup\" label at all is because of the memory\n> leaks. And there the issue is that we build the test helpers with the\n> same compiler settings as the rest of the code, so SANITIZE=leak will\n> complain. So I think that is a non-starter.\n>\n> But if you just meant leaking descriptors, sure, I don't think any tools\n> complain about that. ;)\n\nAt the time the momentary (but not really serious) thought flashed\nthrough my brain, I probably was imagining dropping all of the cleanup\ncode; I almost certainly wasn't thinking about the SANITIZE=leak case.\nBut immediately upon seeing your idea to use die(), I recognized it\n(and liked it) as a superior version of my half-baked thought.\n"},{"id":"522578","messageId":"xmqq5xfiq2ka.fsf@gitster.g","threadId":"63822","inReplyTo":"CAPig+cR-r=CeEaSTeWsX00MLCSRJUUVXMUWS6Ui-HQcR_qMGJA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-23T16:44:53Z","receivedAt":"2025-07-23T16:44:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Jul 22, 2025 at 1:41 PM Hoyoung Lee wrote:\n>> Initialize `fd` to -1 and unify all `open()`-related `close()` calls\n>> under a single cleanup label. This prevents undefined behavior when\n>> `fd` is used without initialization in error paths.\n>\n> It's not clear what this means. As far as I can tell, the original\n> code never used an uninitialized `fd` in error paths.\n>\n>> The cleanup logic now safely avoids calling `close()` on invalid\n>> descriptors and ensures consistent resource management.\n>\n> Again, it's not clear what this means. Although your previous version\n> of this patch did add a call to close() with an invalid descriptor,\n> the original code did not do so. So, the above statement seems to be\n> misleading.\n\nGreat to see that good guidance is offered to new folks. Yes, it is\na great thing to keep in mind to describe the latest iteration for\nreaders who haven't even seen the previous iterations.\n\n> Those issues aside, the patch itself has problems, some minor, such as\n> making the code a bit confusing or misleading, and some major, such as\n> calling close() on an already closed descriptor.\n\n>> data_size = st.st_size;\n>> data_buf = xmalloc(data_size);\n>> if (read_in_full(fd, data_buf, data_size) < 0) {\n>> perror(argv[3]);\n>> goto cleanup;\n>> }\n>> close(fd);\n>\n> The descriptor is closed manually (again) because a subsequent open()\n> call is going to reuse the variable. However...\n>\n>> if (argv[1][1] == 'd')\n>> out_buf = diff_delta(from_buf, from_size,\n>> data_buf, data_size,\n>> &out_size, 0);\n>> else\n>> out_buf = patch_delta(from_buf, from_size,\n>> data_buf, data_size,\n>> &out_size);\n>> if (!out_buf) {\n>> fprintf(stderr, \"delta operation failed (returned NULL)\\n\");\n>> goto cleanup;\n>> }\n>\n> ...although `fd` was closed, it still holds the previously-open\n> non-negative file descriptor, which means that this `goto cleanup`...\n\nGood eyes.\n\n>> fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n>> if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {\n>> perror(argv[4]);\n>> goto cleanup;\n>> }\n>>\n>> ret = 0;\n>> cleanup:\n>> free(from_buf);\n>> free(data_buf);\n>> free(out_buf);\n>>\n>> if (fd >= 0)\n>> close(fd);\n>\n> ...will arrive here and the condition will evaluate to \"true\",\n> resulting in the already-closed descriptor being closed again.\n\nVery clear description.\n\nThanks for your review.\n"},{"id":"522580","messageId":"xmqqwm7yonti.fsf@gitster.g","threadId":"63822","inReplyTo":"20250723075513.GA570540@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-23T16:48:41Z","receivedAt":"2025-07-23T16:48:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I dunno. We are reaching diminishing returns spending brainpower on a\n> function that is meant to be somewhat quick-and-dirty.\n\nTrue. After all, the function is ready to exit and have the system\nclose the file descriptor anyway ;-)\n"},{"id":"522582","messageId":"xmqqms8uomsa.fsf@gitster.g","threadId":"63822","inReplyTo":"CAPig+cQ2p3muA0-8O_u-rOW0uVGQ_ZE7hcz-7qq8M1U+PdvtpQ@mail.gmail.com","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-23T17:11:01Z","receivedAt":"2025-07-23T17:11:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> For completeness, I'll mention that I even had the thought that\n> another \"fix\" would be to tear out all the cleanup code entirely since\n> we _know_ that this function will be exiting immediately and the OS\n> will clean up any dangling resources.\n\n;-)\n"},{"id":"522617","messageId":"20250723235929.GB592873@coredump.intra.peff.net","threadId":"63822","inReplyTo":"20250723080639.GA570920@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-23T23:59:29Z","receivedAt":"2025-07-23T23:59:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 23, 2025 at 04:06:39AM -0400, Jeff King wrote:\n\n> On Wed, Jul 23, 2025 at 03:55:13AM -0400, Jeff King wrote:\n> \n> > I dunno. We are reaching diminishing returns spending brainpower on a\n> > function that is meant to be somewhat quick-and-dirty.\n> \n> OK, I clearly could not resist spending more brainpower on it. If we are\n> doing quick-and-dirty, why not just die()? The end result is the same,\n> but per my argument in the earlier iteration of the series, that means\n> we do not have to worry about cleaning up at all.\n\nSo...I feel a little bad about hijacking Hoyoung's thread. But after all\nof the discussion, it seemed simplest to just stick it all into patches.\nSo here is a potential replacement for patch 2.\n\n [1/3]: test-delta: handle errors with die()\n [2/3]: test-delta: use strbufs to hold input files\n [3/3]: test-delta: close output descriptor after use\n\n t/helper/test-delta.c | 77 ++++++++++++++-----------------------------\n 1 file changed, 24 insertions(+), 53 deletions(-)\n\n-Peff\n"},{"id":"522618","messageId":"20250724000056.GA597294@coredump.intra.peff.net","threadId":"63822","inReplyTo":"20250723235929.GB592873@coredump.intra.peff.net","subject":"[PATCH 1/3] test-delta: handle errors with die()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-24T00:00:56Z","receivedAt":"2025-07-24T00:00:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a short test helper that does all of its work in the main\nfunction. When we encounter an error, we try to clean up memory and\ndescriptors and then jump to an error return, which exits the program.\n\nWe can get the same effect by just calling die(), which means we do not\nhave to bother with cleaning up. This simplifies the code, and also\nremoves some inconsistencies where a few code paths forgot to clean up\ndescriptors (though in practice it was not a big deal since we were\nexiting anyway).\n\nIn addition to die() and die_errno(), we'll also use a few of our usual\nhelpers like xopen() and usage() that make things more ergonomic.\n\nThis does change the exit code in these cases from 1 to 128, but I\ndon't think it matters (and arguably is better, as we'd already exit 128\nfor other errors like xmalloc() failure).\n\nSigned-off-by: Jeff King \n---\n t/helper/test-delta.c | 55 ++++++++++++++-----------------------------\n 1 file changed, 18 insertions(+), 37 deletions(-)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 6bc787a474..4495b32b49 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -21,39 +21,26 @@ int cmd__delta(int argc, const char **argv)\n \tstruct stat st;\n \tvoid *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;\n \tunsigned long from_size, data_size, out_size;\n-\tint ret = 1;\n \n-\tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\"))) {\n-\t\tfprintf(stderr, \"usage: %s\\n\", usage_str);\n-\t\treturn 1;\n-\t}\n+\tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\")))\n+\t\tusage(usage_str);\n \n-\tfd = open(argv[2], O_RDONLY);\n-\tif (fd < 0 || fstat(fd, &st)) {\n-\t\tperror(argv[2]);\n-\t\treturn 1;\n-\t}\n+\tfd = xopen(argv[2], O_RDONLY);\n+\tif (fstat(fd, &st) < 0)\n+\t\tdie_errno(\"fstat(%s)\", argv[2]);\n \tfrom_size = st.st_size;\n \tfrom_buf = xmalloc(from_size);\n-\tif (read_in_full(fd, from_buf, from_size) < 0) {\n-\t\tperror(argv[2]);\n-\t\tclose(fd);\n-\t\tgoto cleanup;\n-\t}\n+\tif (read_in_full(fd, from_buf, from_size) < 0)\n+\t\tdie_errno(\"read(%s)\", argv[2]);\n \tclose(fd);\n \n-\tfd = open(argv[3], O_RDONLY);\n-\tif (fd < 0 || fstat(fd, &st)) {\n-\t\tperror(argv[3]);\n-\t\tgoto cleanup;\n-\t}\n+\tfd = xopen(argv[3], O_RDONLY);\n+\tif (fstat(fd, &st) < 0)\n+\t\tdie_errno(\"fstat(%s)\", argv[3]);\n \tdata_size = st.st_size;\n \tdata_buf = xmalloc(data_size);\n-\tif (read_in_full(fd, data_buf, data_size) < 0) {\n-\t\tperror(argv[3]);\n-\t\tclose(fd);\n-\t\tgoto cleanup;\n-\t}\n+\tif (read_in_full(fd, data_buf, data_size) < 0)\n+\t\tdie_errno(\"read(%s)\", argv[3]);\n \tclose(fd);\n \n \tif (argv[1][1] == 'd')\n@@ -64,22 +51,16 @@ int cmd__delta(int argc, const char **argv)\n \t\tout_buf = patch_delta(from_buf, from_size,\n \t\t\t\t data_buf, data_size,\n \t\t\t\t &out_size);\n-\tif (!out_buf) {\n-\t\tfprintf(stderr, \"delta operation failed (returned NULL)\\n\");\n-\t\tgoto cleanup;\n-\t}\n+\tif (!out_buf)\n+\t\tdie(\"delta operation failed (returned NULL)\");\n \n-\tfd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n-\tif (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {\n-\t\tperror(argv[4]);\n-\t\tgoto cleanup;\n-\t}\n+\tfd = xopen(argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n+\tif (write_in_full(fd, out_buf, out_size) < 0)\n+\t\tdie_errno(\"write(%s)\", argv[4]);\n \n-\tret = 0;\n-cleanup:\n \tfree(from_buf);\n \tfree(data_buf);\n \tfree(out_buf);\n \n-\treturn ret;\n+\treturn 0;\n }\n-- \n2.50.1.666.gdb1e186d6a\n\n"},{"id":"522619","messageId":"20250724000211.GB597294@coredump.intra.peff.net","threadId":"63822","inReplyTo":"20250723235929.GB592873@coredump.intra.peff.net","subject":"[PATCH 2/3] test-delta: use strbufs to hold input files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-24T00:02:11Z","receivedAt":"2025-07-24T00:02:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We want to read the whole contents of two files into memory. If we\nswitch from raw ptr/len pairs to strbufs, we can use strbuf_read_file()\nto shorten the code.\n\nThis incidentally fixes two small bugs:\n\n 1. We stat() the files and allocate our buffers based on st.st_size.\n But that is an off_t which may be larger than the size_t we'd use\n to allocate. We should use xsize_t() to do a checked conversion.\n Otherwise integer truncation (on a file >4GB) could cause us to\n under-allocate (though in practice this does not result in a buffer\n overflow because the same truncation happens when read_in_full()\n also takes a size_t).\n\n 2. We get the size from st.st_size, and then try to read_in_full()\n that many bytes. But it may return fewer bytes than expected (if\n the file changed racily and we get an early EOF), leading us to\n read uninitialized bytes in the allocated buffer. We don't notice\n because we only check the value for error, not that we got the\n expected number of bytes.\n\nThe strbuf code doesn't run into this, because it just reads to EOF,\nexpanding the buffer dynamically as necessary. Neither bug is a big deal\nfor a test helper, but fixing them is a nice bonus on top of simplifying\nthe code.\n\nSigned-off-by: Jeff King \n---\n t/helper/test-delta.c | 40 ++++++++++++++--------------------------\n 1 file changed, 14 insertions(+), 26 deletions(-)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 4495b32b49..7945793078 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -11,45 +11,33 @@\n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n #include \"delta.h\"\n+#include \"strbuf.h\"\n \n static const char usage_str[] =\n \t\"test-tool delta (-d|-p) \";\n \n int cmd__delta(int argc, const char **argv)\n {\n \tint fd;\n-\tstruct stat st;\n-\tvoid *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;\n-\tunsigned long from_size, data_size, out_size;\n+\tstruct strbuf from = STRBUF_INIT, data = STRBUF_INIT;\n+\tchar *out_buf;\n+\tunsigned long out_size;\n \n \tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\")))\n \t\tusage(usage_str);\n \n-\tfd = xopen(argv[2], O_RDONLY);\n-\tif (fstat(fd, &st) < 0)\n-\t\tdie_errno(\"fstat(%s)\", argv[2]);\n-\tfrom_size = st.st_size;\n-\tfrom_buf = xmalloc(from_size);\n-\tif (read_in_full(fd, from_buf, from_size) < 0)\n-\t\tdie_errno(\"read(%s)\", argv[2]);\n-\tclose(fd);\n-\n-\tfd = xopen(argv[3], O_RDONLY);\n-\tif (fstat(fd, &st) < 0)\n-\t\tdie_errno(\"fstat(%s)\", argv[3]);\n-\tdata_size = st.st_size;\n-\tdata_buf = xmalloc(data_size);\n-\tif (read_in_full(fd, data_buf, data_size) < 0)\n-\t\tdie_errno(\"read(%s)\", argv[3]);\n-\tclose(fd);\n+\tif (strbuf_read_file(&from, argv[2], 0) < 0)\n+\t\tdie_errno(\"unable to read '%s'\", argv[2]);\n+\tif (strbuf_read_file(&data, argv[3], 0) < 0)\n+\t\tdie_errno(\"unable to read '%s'\", argv[3]);\n \n \tif (argv[1][1] == 'd')\n-\t\tout_buf = diff_delta(from_buf, from_size,\n-\t\t\t\t data_buf, data_size,\n+\t\tout_buf = diff_delta(from.buf, from.len,\n+\t\t\t\t data.buf, data.len,\n \t\t\t\t &out_size, 0);\n \telse\n-\t\tout_buf = patch_delta(from_buf, from_size,\n-\t\t\t\t data_buf, data_size,\n+\t\tout_buf = patch_delta(from.buf, from.len,\n+\t\t\t\t data.buf, data.len,\n \t\t\t\t &out_size);\n \tif (!out_buf)\n \t\tdie(\"delta operation failed (returned NULL)\");\n@@ -58,8 +46,8 @@ int cmd__delta(int argc, const char **argv)\n \tif (write_in_full(fd, out_buf, out_size) < 0)\n \t\tdie_errno(\"write(%s)\", argv[4]);\n \n-\tfree(from_buf);\n-\tfree(data_buf);\n+\tstrbuf_release(&from);\n+\tstrbuf_release(&data);\n \tfree(out_buf);\n \n \treturn 0;\n-- \n2.50.1.666.gdb1e186d6a\n\n"},{"id":"522620","messageId":"20250724000308.GC597294@coredump.intra.peff.net","threadId":"63822","inReplyTo":"20250723235929.GB592873@coredump.intra.peff.net","subject":"[PATCH 3/3] test-delta: close output descriptor after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-24T00:03:08Z","receivedAt":"2025-07-24T00:03:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After we write to the output file, the program exits. This naturally\ncloses the descriptor. But we should do an explicit close for two\nreasons:\n\n 1. It's possible to hit an error on close(), which we should detect\n and report via our exit code.\n\n 2. Leaking descriptors is a bad practice in general. Even if it isn't\n meaningful here, it sets a bad example.\n\nIt is tempting to write:\n\n if (write_in_full(fd, ...) < 0 || close(fd) < 0)\n die_errno(...);\n\nBut that pattern contains a subtle problem that has resulted in\ndescriptor leaks before. If write_in_full() fails, we'll short-circuit\nand never call close(), leaking the descriptor.\n\nThat's not a problem here, since our error path dies instead of\nreturning up the stack. But since we're trying to set a good example,\nlet's write it out as two separate conditions. As a bonus, that lets us\nproduce a slightly more specific error message.\n\nSigned-off-by: Jeff King \n---\nAnother option is that the program should just write to stdout, then we\ndo not have to open or close it at all. ;)\n\n t/helper/test-delta.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 7945793078..52ea00c937 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -45,6 +45,8 @@ int cmd__delta(int argc, const char **argv)\n \tfd = xopen(argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n \tif (write_in_full(fd, out_buf, out_size) < 0)\n \t\tdie_errno(\"write(%s)\", argv[4]);\n+\tif (close(fd) < 0)\n+\t\tdie_errno(\"close(%s)\", argv[4]);\n \n \tstrbuf_release(&from);\n \tstrbuf_release(&data);\n-- \n2.50.1.666.gdb1e186d6a\n"}]}