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

From: Caleb White <cdwhite3@pm.me>
Date: Oct 6, 2024, 23:03 UTC
Message-ID: <VKrLGkXtD_CWHlQJRx_JqPn9sFsjKL88fRnGvA2UcV_gvQxyv8kmIx9M6BkPRfAx9paB7KrjkL9XAE8P2P2EPJnciBN4F6LAAc176NzLh6U=@pm.me>
In-Reply-To: <CAPig+cSdVVy4huueVQpiO_Gvn4SAXAiQj-uVnuScgfOOFJ6h0g@mail.gmail.com>

On Sunday, October 6th, 2024 at 06:24, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 5 quoted lines

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

When I was working through different solution for the 3rd patch, I encountered this issue and this was the fix for that (granted, I could've handled this on the caller side). It turned out that I had to go a different direction and this was no longer needed, but I figured it wouldn't hurt to leave this in which is why it's the last patch. However, I can just drop this if you want.

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

Sometimes it's cleaner to write a check once on the callee side instead of all callers having to duplicate the same check. This also follows the same pattern as free_worktree():

```
void free_worktree(struct worktree *worktree)
{
	if (!worktree)
		return;
```

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

Will do (unless we decide to drop this).

Show 7 quoted lines

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

I didn't change this originally, but then the build process threw errors that I had code before declarations (because I placed the if condition at the start of the function). I could've moved the if condition past the declaration, but it seemed weird to need to declare a variable if the function is just going to immediately return due to a NULL pointer.

If we decide to keep this patch then I can keep the separate declaration.