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

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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.

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

Good eyes.

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