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

Eric Sunshine [off-list ref] writes:

On Tue, Jul 22, 2025 at 1:41 PM Hoyoung Lee [off-list ref] wrote:

quoted

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.

quoted

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.

Great to see that good guidance is offered to new folks.  Yes, it is
a great thing to keep in mind to describe the latest iteration for
readers who haven't even seen the previous iterations.

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.

quoted

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

quoted

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

Good eyes.

quoted

  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.

Very clear description.

Thanks for your review.