From: Eric Sunshine <sunshine@sunshineco.com>
Date: Tue, 22 Jul 2025 08:36:14 GMT
Subject: Re: [PATCH v2 3/4] t/helper/test-delta: close fd if fstat() fails after open()
Message-ID: <CAPig+cT1hzz4_7JqesqFmgYdpsNezurcsF9XF1ep8KzvgMUc2Q@mail.gmail.com>
In-Reply-To: <20250722081219.1086866-4-lhywkd22@gmail.com>

On Tue, Jul 22, 2025 at 4:12 AM Hoyoung Lee <lhywkd22@gmail.com> 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@gmail.com>
> ---
> 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.