Re: [PATCH v3 2/2] t/helper/test-delta: fix possible resource leak and ensure safe cleanup

[Eric Sunshine <sunshine@sunshineco.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...

>   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.

>   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.

>   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...

>   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`...

>   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;