Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jul 23, 2025, 07:28 UTC
- Message-ID
- <CAPig+cR-r=CeEaSTeWsX00MLCSRJUUVXMUWS6Ui-HQcR_qMGJA@mail.gmail.com>
- In-Reply-To
- <20250722174102.1876197-3-lhywkd22@gmail.com>
On Tue, Jul 22, 2025 at 1:41 PM Hoyoung Lee <lhywkd22@gmail.com> wrote:
> Initialize `fd` to -1 and unify all `open()`-related `close()` calls > under a single cleanup label. This prevents undefined behavior when > `fd` is used without initialization in error paths.
It's not clear what this means. As far as I can tell, the original code never used an uninitialized `fd` in error paths.
> The cleanup logic now safely avoids calling `close()` on invalid > descriptors and ensures consistent resource management.
Again, it's not clear what this means. Although your previous version of this patch did add a call to close() with an invalid descriptor, the original code did not do so. So, the above statement seems to be misleading.
Those issues aside, the patch itself has problems, some minor, such as making the code a bit confusing or misleading, and some major, such as calling close() on an already closed descriptor.
Unfortunately, there are enough important context lines missing from the patch itself that, instead of directly reviewing the patch directly, I'm going to review the code following the application of your patch...
> int fd = -1;
This new initialization (-1) is useless because...
Show 6 quoted lines
> if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) {
> fprintf(stderr, "usage: %s\n", usage_str);
> return 1;
> }
>
> fd = open(argv[2], O_RDONLY);...the very first time `fd` is mentioned (aside from the declaration) is here where it is unconditionally assigned a value. Thus, the -1 initialization is wasted (and potentially confusing for readers).
> if (fd < 0 || fstat(fd, &st)) {
> perror(argv[2]);
> goto cleanup;
> }Okay, no problem here. The `if (fd >= 0) close(fd)` you added to the "cleanup" action handles both the cases here when `fd` might be negative after the open() call or a valid descriptor.
Show 7 quoted lines
> from_size = st.st_size;
> from_buf = xmalloc(from_size);
> if (read_in_full(fd, from_buf, from_size) < 0) {
> perror(argv[2]);
> goto cleanup;
> }
> close(fd);Here `fd` is closed manually which is good because...
> fd = open(argv[3], O_RDONLY);
...this code immediately assigns it a new value.
> if (fd < 0 || fstat(fd, &st)) {
> perror(argv[3]);
> goto cleanup;
> }Okay for the same reason mentioned above.
Show 7 quoted lines
> data_size = st.st_size;
> data_buf = xmalloc(data_size);
> if (read_in_full(fd, data_buf, data_size) < 0) {
> perror(argv[3]);
> goto cleanup;
> }
> close(fd);The descriptor is closed manually (again) because a subsequent open() call is going to reuse the variable. However...
Show 12 quoted lines
> if (argv[1][1] == 'd')
> out_buf = diff_delta(from_buf, from_size,
> data_buf, data_size,
> &out_size, 0);
> else
> 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;
> }...although `fd` was closed, it still holds the previously-open non-negative file descriptor, which means that this `goto cleanup`...
Show 14 quoted lines
> 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;
> }
>
> ret = 0;
> cleanup:
> free(from_buf);
> free(data_buf);
> free(out_buf);
>
> if (fd >= 0)
> close(fd);...will arrive here and the condition will evaluate to "true", resulting in the already-closed descriptor being closed again.
> return ret;