Re: [PATCH] revision: fix missing null for freed memory

From: Jeff King <peff@peff.net>
Date: Feb 11, 2025, 21:29 UTC
Message-ID: <20250211212909.GA3113114@coredump.intra.peff.net>
In-Reply-To: <CALnO6CDdJ4abqxZKMaevPO+aCzSqriM98JuVOX068gQrxWZt5Q@mail.gmail.com>

On Tue, Feb 11, 2025 at 03:22:28PM -0500, D. Ben Knoble wrote:

Show 12 quoted lines

> 2.{30,35}.0 fails to recognize --no-graph, so I checked "git log --grep no-graph
> origin/master" with "git describe --contains" and decided that 2.36.0 was first
> release recognizing --no-graph, but it didn't build for me (possibly an issue on
> my end). I got 2.37.0 built, and it was "good," so that's where I started.
> 
> Here's my "bisect run" script.
> 
>     #! /bin/sh -x
>     make || exit 125
>     # segfault has exit >128
>     ./bin-wrappers/git --no-pager log -2 --graph --no-graph --patch
> --cc || exit 1

I don't think this is quite enough. The problem is a use-after-free, so the behavior is undefined. Depending on whether that heap block is reused, it might work just fine, or output garbage data, or segfault.

I'd have _thought_ it would usually just segfault, but it almost always just output garbage for me. Building with:

  make SANITIZE=address,undefined

is a good way to get reliable results for this kind of memory error. Doing that shows that v2.37.0 is actually bad. And bisecting shows that it has been broken since 087c745833 (log: add a --no-graph option, 2022-02-11), which is not too surprising.

> The --cc is important, since this repro logs from where the bisect is! Without
> it, if the head commits are both merges (likely), the repro will accidentally
> mark the commit as good when looking further for a commit with a patch will
> fail. Omitting -2 might work, too, but that makes "git log" take longer.

I've also run into non-determinism when bisecting like this, because my test command depends on the value of HEAD. The best solution here is to just feed a stable tip to git-log. I bisected on:

  git log --graph --no-graph --patch origin >/dev/null

(I didn't need "-2" because good commits failed with "unrecognized argument" and bad ones were killed by ASan immediately ;) ).

-Peff