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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

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

Attachment: signature.asc
Description: OpenPGP digital signature


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux