Re: [PATCH v2 4/4] worktree: prevent null pointer dereference

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 6, 2024, 11:24 UTC
Message-ID: <CAPig+cSdVVy4huueVQpiO_Gvn4SAXAiQj-uVnuScgfOOFJ6h0g@mail.gmail.com>
In-Reply-To: <20241006060017.171788-5-cdwhite3@pm.me>

On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:

> If worktrees is NULL, free_worktrees() should return immediately to
> prevent a null pointer dereference.
>
> Signed-off-by: Caleb White <cdwhite3@pm.me>

Critical questions: It is not clear why this patch is needed, especially coming at the end of the series. Is there some code in a patch earlier in the series which might call free_worktrees() with NULL? If so, then this patch should come before that patch. If not, then why do we need this patch at all?

Devil's advocate question: Why is it the responsibility of free_worktrees() to check this condition as opposed to being the caller's responsibility?

The commit message should explain the need for this patch and answer these questions, not just say what change is being made.

Show 10 quoted lines

> diff --git a/worktree.c b/worktree.c
> @@ -28,8 +28,9 @@ void free_worktree(struct worktree *worktree)
>  void free_worktrees(struct worktree **worktrees)
>  {
> -       int i = 0;
> -       for (i = 0; worktrees[i]; i++)
> +       if  (!worktrees)
> +               return;
> +       for (int i = 0; worktrees[i]; i++)
>                 free_worktree(worktrees[i]);

Although it's true that this project has recently started allowing declaration of the variable in the `for` statement, that change is unrelated to the stated purpose in the commit message. True, it's a minor thing in this case, but it causes a hiccup for reviewers when unrelated changes are piggybacked like this with the "real" change since it adds noise which obscures what the reviewer should really be focusing on.