# [BUG] revision: premature free-and-null causes “unknown option `(null)`” 9 messages from 2026-09-25 to 2026-09-29. Participants: Kristoffer Haugsbakk, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/66390 ## Kristoffer Haugsbakk, 2026-09-25 00:28 Subject: [BUG] revision: premature free-and-null causes “unknown option `(null)`” Message-ID: <74796901-ffb1-4cf3-bd63-7294328f70bc@app.fastmail.com> ``` (the subject is my preliminary speculation) Thank you for filling out a Git bug report! Please answer the following questions to help us understand your issue. What did you do before the bug happened? (Steps to reproduce your issue) ˋˋˋ git shortlog -n --not-an-option master ˋˋˋ Note that you need a real option like `-n` before the `--not-an-option`. Or else it will work correctly. What did you expect to happen? (Expected behavior) This error: ˋˋˋ error: unknown option `--not-an-option' [usage printout] ˋˋˋ What happened instead? (Actual behavior) This error: ˋˋˋ error: unknown option `(null)' [usage printout] ˋˋˋ What's different between what you expected and what actually happened? I expected it to print the option in quotes. Instead it printed `(null)` which I think is the placeholder for when the `%s` arg is `NULL`. Anything else you want to add: I have tested and reproduced on: • master: 0f8e75ab (Revert "Merge branch 'en/no-amend-during-conflicts'", 2026-09-23) • seen: e844e042 (Merge branch 'je/doc-merge-conflicts' into seen, 2026-09-24) • next: d58861e6 (Revert "Merge branch 'gg/http-ssl-verify-status' into next", 2026-09-23) I have bisected this to cd439487 (revision: manage memory ownership of argv in setup_revisions(), 2025-09-19). Note that the recent topic jk/rev-info-argv-to-free fixes issues caused by commit cd439487, but that topic does not change how this option handling behaves; the topic is part of `master` now which I tested. I also tested on top of the topic and got the same `(null)' result. Please review the rest of the bug report below. You can delete any lines you don't wish to share. [System Info] git version: git version 2.55.0.793.gc667de3f2c5 cpu: x86_64 built from commit: c667de3f2c5e43830a8dfaa79a27d5d1f106f0ab sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh rust: enabled feature: fsmonitor--daemon gettext: enabled libcurl: 7.81.0 OpenSSL: OpenSSL 3.0.2 15 Mar 2022 zlib: 1.2.11 SHA-1: SHA1_DC SHA-256: SHA256_BLK default-ref-format: files default-hash: sha1 uname: Linux 6.8.0-138-generic #138~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Aug 7 13:43:15 UTC x86_64 compiler info: gnuc: 11.4 libc info: glibc: 2.35 $SHELL (typically, interactive shell): /bin/bash [Enabled Hooks] commit-msg post-applypatch post-checkout post-commit sendemail-validate -- Kristoffer Haugsbakk kristofferhaugsbakk@fastmail.com ``` ## Jeff King, 2026-09-25 08:26 Subject: Re: [BUG] revision: premature free-and-null causes “unknown option `(null)`” Message-ID: <20260925082636.GA1493716@coredump.intra.peff.net> In-Reply-To: <74796901-ffb1-4cf3-bd63-7294328f70bc@app.fastmail.com> ``` On Fri, Sep 25, 2026 at 02:28:57AM +0200, Kristoffer Haugsbakk wrote: > git shortlog -n --not-an-option master > [...] > > I expected it to print the option in quotes. Instead it printed `(null)` > which I think is the placeholder for when the `%s` arg is `NULL`. > [...] > > I have bisected this to cd439487 (revision: manage memory ownership of > argv in setup_revisions(), 2025-09-19). Yep, definitely my fault. I don't have time to do a full write-up now, but the most direct solution is: diff --git a/revision.c b/revision.c index ee1df92d1d..501a4ba36e 100644 --- a/revision.c +++ b/revision.c @@ -2768,13 +2768,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, const struct option *options, const char * const usagestr[]) { int n = handle_revision_opt(revs, ctx->argc, ctx->argv, &ctx->cpidx, ctx->out, NULL); if (n <= 0) { - error("unknown option `%s'", ctx->argv[0]); + error("unknown option `%s'", ctx->out[ctx->cpidx - 1]); usage_with_options(usagestr, options); } ctx->argv += n; ctx->argc -= n; } But I think instead doing this: diff --git a/revision.c b/revision.c index ee1df92d1d..7b858d54c1 100644 --- a/revision.c +++ b/revision.c @@ -2340,7 +2340,8 @@ static void overwrite_argv(int *argc, const char **argv, if (*value != argv[*argc]) { mark_argv_for_free(opt, revs, argv[*argc]); argv[*argc] = *value; - *value = NULL; + if (opt && opt->free_removed_argv_elements) + *value = NULL; } (*argc)++; } will restore some of the hidden assumptions made by pre-cd439487 code. So it would fix this case, along with any other lurkers. I know that's probably quite opaque. ;) I'll fully explain what's going on in a follow-up tomorrow, but I wanted to post the solution quickly so nobody else wasted time digging. Thanks for a clear report. -Peff ``` ## Jeff King, 2026-09-25 20:33 Subject: [PATCH 0/2] some parse_revision_opt() bugfixes Message-ID: <20260925203359.GA1506705@coredump.intra.peff.net> In-Reply-To: <20260925082636.GA1493716@coredump.intra.peff.net> ``` On Fri, Sep 25, 2026 at 04:26:36AM -0400, Jeff King wrote: > > I have bisected this to cd439487 (revision: manage memory ownership of > > argv in setup_revisions(), 2025-09-19). > > Yep, definitely my fault. I don't have time to do a full write-up now, > but the most direct solution is: > [...] > But I think instead doing this: I ended up reversing my decision there, for reasons that are explained in the second commit. But the good news is that doing so also revealed a related bug that predates even cd439487. So here are the fixes. I apologize in advance for the length of the second commit message. At least I feel good about my decision to go to bed before writing it. ;) [1/2]: revision: avoid reporting known options as unknown on error [2/2]: revision: handle argv movement in parse_revision_opt() revision.c | 7 +++++-- t/t4201-shortlog.sh | 11 +++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) -Peff ``` ## Jeff King, 2026-09-25 20:35 Subject: [PATCH 1/2] revision: avoid reporting known options as unknown on error Message-ID: <20260925203548.GA1544493@coredump.intra.peff.net> In-Reply-To: <20260925203359.GA1506705@coredump.intra.peff.net> ``` In parse_revision_opt(), we report "unknown option" when handle_revision_opt() returns a negative value or zero. But these are two different conditions: a negative value indicates a malformed option (like a missing argument) and zero indicates an unknown option. So malformed options produce nonsense like this: $ git shortlog --default error: bad --default argument error: unknown option `--default' usage: [...] We should treat a negative return as a logic error which has already been reported by handle_revision_opt(), and just show the regular usage message. Signed-off-by: Jeff King --- There's obviously a way to write this that makes the diff a little shorter and doesn't repeat the usage_with_options() line. I think the if/else cascade makes the mental model more clear, though. revision.c | 5 ++++- t/t4201-shortlog.sh | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index ee1df92d1d..f958d8c301 100644 --- a/revision.c +++ b/revision.c @@ -2771,7 +2771,10 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, { int n = handle_revision_opt(revs, ctx->argc, ctx->argv, &ctx->cpidx, ctx->out, NULL); - if (n <= 0) { + if (n < 0) { + /* handle_revision_opt() has already reported the error. */ + usage_with_options(usagestr, options); + } else if (!n) { error("unknown option `%s'", ctx->argv[0]); usage_with_options(usagestr, options); } diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh index 023fbff546..4ba7f5aec6 100755 --- a/t/t4201-shortlog.sh +++ b/t/t4201-shortlog.sh @@ -436,4 +436,10 @@ test_expect_success 'stdin with multiple groups reports error' ' test_must_fail git shortlog --group=author --group=committer err && + test_grep "bad --default argument" err && + test_grep ! "unknown option" err +' + test_done -- 2.56.0.rc2.289.g137cf50cac ``` ## Jeff King, 2026-09-25 20:39 Subject: [PATCH 2/2] revision: handle argv movement in parse_revision_opt() Message-ID: <20260925203958.GB1544493@coredump.intra.peff.net> In-Reply-To: <20260925203359.GA1506705@coredump.intra.peff.net> ``` The argument parser used by setup_revisions() modifies the argv array that is passed to it, consolidating non-options and unknown options at the start of the array. This led to problems with memory leaks when argv pointed to allocated strings. We addressed that in cd43948798 (revision: manage memory ownership of argv in setup_revisions(), 2025-09-19). Now instead of copying strings to the earlier part of argv, we actually move them, setting the original location to NULL (so that we know we have exactly one pointer to the string). This works fine for setup_revisions() itself, but the underlying handle_revision_opt() has another entry point: parse_revision_opt(). This lets a parse-options user parse a single revision option, but the movement introduced by cd43948798 confuses its error code path. If we see an unknown option, then handle_revision_opt() will move it out of the way (to the "unknown options" section) and return an error. But parse_revision_opt() then tries to access the original argv location, which has now been set to NULL, and you get: $ git shortlog -n --no-such-option error: unknown option `(null)' Whereas prior to cd43948798, it would have been a leftover copy of the pointer (that may or may not eventually get written over, but was valid for this immediate message). And you get what you'd expect: $ git shortlog -n --no-such-option error: unknown option `--no-such-option' Making things even more confusing, it only happens if there's another option before the unknown one! That's because with just: $ git shortlog --no-such-option we "consolidate" to the exact same spot, and no movement occurs at all. Note that we use shortlog in these examples because it is one of only two commands that use the parse_revision_opt() interface (the other is blame). There are a few options for fixing this. One is that we can observe that the "move" semantics introduced by cd43948798 only matter when the argv strings are allocated on the heap, in which case the caller passes in the free_removed_argv_elements flag to tell us. But we never use that flag with parse_revision_opt(). So we could do something like this: diff --git a/revision.c b/revision.c index ee1df92d1d..7b858d54c1 100644 --- a/revision.c +++ b/revision.c @@ -2340,7 +2340,8 @@ static void overwrite_argv(int *argc, const char **argv, if (*value != argv[*argc]) { mark_argv_for_free(opt, revs, argv[*argc]); argv[*argc] = *value; - *value = NULL; + if (opt && opt->free_removed_argv_elements) + *value = NULL; } (*argc)++; } to restore the pre-cd43948798 semantics when heap-allocated strings are not in use. We'd just keep the extra pointer in the original location, but nobody cares because they're not going to free anything anyway. That's enough to fix this case, and could fix any other theoretical cases we haven't noticed. The downside is that it's an accident waiting to happen if we ever do teach parse_revision_opt() to handle allocated argv strings. But are there other theoretical cases? I don't think so. The code paths touched by cd43948798 are either in setup_revisions() itself (which also learned how to handle this movement) or in handle_revision_opt(), the low-level static helper. It has only two callers: setup_revisions() itself, and parse_revision_opt() in which we see the current breakage. So fixing parse_revision_opt() should cover all of our bases, and keep the code ready for a potential future change to handle allocated strings. The fix is just to tell parse_revision_opt() to look for the unknown option in the consolidated destination rather than the original location. We might write to that consolidated location for other reasons (like moving pseudo-revision options like "--all"), but there is only one code path that returns the 0 for an unknown option, and it always moves the option before doing so. So the "end" of that consolidated area will always have our unknown option. This patch implements that solution and demonstrates the breakage and fix using shortlog. Reported-by: Kristoffer Haugsbakk Signed-off-by: Jeff King --- Obviously another possible fix is for parse_revision_opt() to record the string before passing it along, and use that for its error message. That seemed clunkier to me. revision.c | 2 +- t/t4201-shortlog.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index f958d8c301..a83e499047 100644 --- a/revision.c +++ b/revision.c @@ -2775,7 +2775,7 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, /* handle_revision_opt() has already reported the error. */ usage_with_options(usagestr, options); } else if (!n) { - error("unknown option `%s'", ctx->argv[0]); + error("unknown option `%s'", ctx->out[ctx->cpidx - 1]); usage_with_options(usagestr, options); } ctx->argv += n; diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh index 4ba7f5aec6..10c43e6e75 100755 --- a/t/t4201-shortlog.sh +++ b/t/t4201-shortlog.sh @@ -442,4 +442,9 @@ test_expect_success 'invalid revision options are not reported as unknown' ' test_grep ! "unknown option" err ' +test_expect_success 'unknown revision options are reported correctly' ' + test_must_fail git shortlog -n --no-such-option 2>err && + test_grep "unknown option .*--no-such-option" err +' + test_done -- 2.56.0.rc2.289.g137cf50cac ``` ## Junio C Hamano, 2026-09-25 22:23 Subject: Re: [PATCH 0/2] some parse_revision_opt() bugfixes Message-ID: In-Reply-To: <20260925203359.GA1506705@coredump.intra.peff.net> ``` Jeff King writes: > On Fri, Sep 25, 2026 at 04:26:36AM -0400, Jeff King wrote: > >> > I have bisected this to cd439487 (revision: manage memory ownership of >> > argv in setup_revisions(), 2025-09-19). >> >> Yep, definitely my fault. I don't have time to do a full write-up now, >> but the most direct solution is: >> [...] >> But I think instead doing this: > > I ended up reversing my decision there, for reasons that are explained > in the second commit. But the good news is that doing so also revealed a > related bug that predates even cd439487. > > So here are the fixes. I apologize in advance for the length of the > second commit message. At least I feel good about my decision to go to > bed before writing it. ;) > > [1/2]: revision: avoid reporting known options as unknown on error > [2/2]: revision: handle argv movement in parse_revision_opt() > > revision.c | 7 +++++-- > t/t4201-shortlog.sh | 11 +++++++++++ > 2 files changed, 16 insertions(+), 2 deletions(-) Both patches make sense. : git; rungit v2.52.0 shortlog -n --no-such-option 2>&1 | head -n1 error: unknown option `(null)' : git; rungit v2.51.0 shortlog -n --no-such-option 2>&1 | head -n1 error: unknown option `--no-such-option' : git; ./git shortlog -n --no-such-option 2>&1 | head -n1 error: unknown option `--no-such-option' ``` ## Kristoffer Haugsbakk, 2026-09-26 09:00 Subject: Re: [PATCH 2/2] revision: handle argv movement in parse_revision_opt() Message-ID: In-Reply-To: <20260925203958.GB1544493@coredump.intra.peff.net> ``` On Fri, Sep 25, 2026, at 22:39, Jeff King wrote: > The argument parser used by setup_revisions() modifies the argv array > that is passed to it, consolidating non-options and unknown options at > the start of the array. This led to problems with memory leaks when argv > pointed to allocated strings. We addressed that in cd43948798 (revision: > manage memory ownership of argv in setup_revisions(), 2025-09-19). Now > instead of copying strings to the earlier part of argv, we actually move > them, setting the original location to NULL (so that we know we have > exactly one pointer to the string). > >[snip] > > Reported-by: Kristoffer Haugsbakk I personally prefer the email that I use for commits: (Which has always been the case. But I didn’t want to disrupt the process previously.) I ought to send in a `.mailmap` change with my canonical email address. >[snip] > +test_expect_success 'unknown revision options are reported correctly' ' > + test_must_fail git shortlog -n --no-such-option 2>err && > + test_grep "unknown option .*--no-such-option" err > +' Just thinking this through. This is a regression test indirectly related to git-shortlog(1). So the test does not name `shortlog`, so that’s good. The subtlety of the previously discussed: Making things even more confusing, it only happens if there's another option before the unknown one! is not obvious from the test description, but one can surmise that it is needed since it’s there in the first place. For the next readers of this test suite that come along, it might seem strange that this specific sequence is tested for, and on a shortlog test suite. But it seems normal in this project to add tests that, in the context of the file alone, might not be obvious why they are there (because they are regression tests for very specific bugs). I could imagine some system where regression tests are marked with some identifier that however indirectly links back to whatever triggered the fix. But for one, this would be a new system/convention and wouldn’t make sense to use on just one test. And second, this would just make it more directly accessible; it is still directly accessible for people who know how to query git(1). Well, maybe more indirectly as time goes on if the test is changed and you use the “pickaxe” technique. This is all to say that this test makes sense as it is written now. > + > test_done > -- > 2.56.0.rc2.289.g137cf50cac Thanks for fixing. :) ``` ## Jeff King, 2026-09-28 03:25 Subject: Re: [PATCH 2/2] revision: handle argv movement in parse_revision_opt() Message-ID: <20260928032553.GA493672@coredump.intra.peff.net> In-Reply-To: ``` On Sat, Sep 26, 2026 at 11:00:05AM +0200, Kristoffer Haugsbakk wrote: > > Reported-by: Kristoffer Haugsbakk > > I personally prefer the email that I use for commits: > > > > (Which has always been the case. But I didn’t want to disrupt the > process previously.) OK. I pulled it from your From header, of course. :) > I ought to send in a `.mailmap` change with my canonical email address. We don't mailmap trailers, though. I have a patch to let you do so with %(trailers:mailmap), but you'd still see the original most of the time (since git-log, etc, just dump the raw contents and expect the trailers to be readable). > > +test_expect_success 'unknown revision options are reported correctly' ' > > + test_must_fail git shortlog -n --no-such-option 2>err && > > + test_grep "unknown option .*--no-such-option" err > > +' > > Just thinking this through. This is a regression test indirectly related > to git-shortlog(1). So the test does not name `shortlog`, so that’s good. > The subtlety of the previously discussed: > > Making things even more confusing, it only happens if there's > another option before the unknown one! > > is not obvious from the test description, but one can surmise that it is > needed since it’s there in the first place. Yeah. I sort of assume that anybody wondering about the details of a line of code in this project will be able to dig around with blame or pickaxe. Perhaps a comment could help, but I think anything beyond "it is important that there are two options here" would end up re-hashing the whole explanation in the commit message. > For the next readers of this test suite that come along, it might seem > strange that this specific sequence is tested for, and on a shortlog > test suite. But it seems normal in this project to add tests that, in > the context of the file alone, might not be obvious why they are there > (because they are regression tests for very specific bugs). I could > imagine some system where regression tests are marked with some > identifier that however indirectly links back to whatever triggered the > fix. But for one, this would be a new system/convention and wouldn’t > make sense to use on just one test. And second, this would just make it > more directly accessible; it is still directly accessible for people who > know how to query git(1). Well, maybe more indirectly as time goes on if > the test is changed and you use the “pickaxe” technique. Yeah, exactly. Both patches are really bugs in the revision / parse-options integration function that just happens to be triggerable by shortlog. Possibly something like t0040 would make sense, but it feels weird to be sticking a shortlog invocation there. I dunno. Again, I sort of rely on people to find the relevant commits. > This is all to say that this test makes sense as it is written now. Thanks! -Peff ``` ## Kristoffer Haugsbakk, 2026-09-29 07:43 Subject: Re: [PATCH 2/2] revision: handle argv movement in parse_revision_opt() Message-ID: <7ffbbf0b-4954-4ee8-863c-4ca89ae662c6@app.fastmail.com> In-Reply-To: <20260928032553.GA493672@coredump.intra.peff.net> ``` On Mon, Sep 28, 2026, at 05:25, Jeff King wrote: > On Sat, Sep 26, 2026 at 11:00:05AM +0200, Kristoffer Haugsbakk wrote: >> I ought to send in a `.mailmap` change with my canonical email address. > > We don't mailmap trailers, though. [...] Yes, that’s the reason. There’s no dynamic remapping after the fact. But with a mailmap entry you can ask git-check-mailmap(1) if you have the correct entry while writing or normalizing the commit. I had use for that once when I was collecting `Reported-by`. One report was from three years prior. But in the meantime, this person had changed their address. But they had recorded it in the `.mailmap`. But for such a normalization to be workable at all I would have to try to add something like `--evaluate-all-cmds` to git-interpret- trailers(1). Because `trailer..cmd` will not evaluate trailer values if it is just reading in a file without any `--trailer` options (as well). *** Which is an itch that no one else has to care about. ```