On Tue, Jul 22, 2025 at 4:12 AM Hoyoung Lee <lhywkd22@xxxxxxxxx> wrote:
> If open() succeeds but fstat() fails, the file descriptor is not
> closed, causing a resource leak. This patch adds a close(fd) call
> in the failure path after fstat() to ensure proper resource cleanup.
>
> Signed-off-by: Hoyoung Lee <lhywkd22@xxxxxxxxx>
> ---
> diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
> @@ -31,6 +31,7 @@ int cmd__delta(int argc, const char **argv)
> fd = open(argv[2], O_RDONLY);
> if (fd < 0 || fstat(fd, &st)) {
> perror(argv[2]);
> + close(fd);
> return 1;
> }
One condition under which this block is entered is if `fd` is less
than 0, which means close() is now being called with a negative file
descriptor, which seems quite suspect. I'd think you would want to
either restructure it into two `if` statements:
if (fd < 0) {
...
return 1;
}
if (fstat(fd, ...)) {
...
return 1;
}
or at least protect the call to close():
if (fd < 0 || fstat(fd, ...)) {
...
if (fd >= 0)
close(fd);
return 1;
}
I think the separate `if` statements are probably a bit easier to
reason about, but it is of course subjective.
