Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> On Tue, Jul 22, 2025 at 1:41 PM Hoyoung Lee <lhywkd22@xxxxxxxxx> 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.
