# [PATCH 0/2] Fix a segfault in git log --notes 5 messages from 2020-11-23 to 2020-11-23. Participants: nate@roosteregg.cc, Junio C Hamano. Thread: https://gitlist.dev/t/54692 ## nate@roosteregg.cc, 2020-11-23 03:23 Subject: [PATCH 0/2] Fix a segfault in git log --notes Message-ID: <20201123032342.24566-1-nate@roosteregg.cc> URL: https://gitlist.dev/e/20201123032342.24566-1-nate%40roosteregg.cc ``` From: Nate Avers Hi, I came across a segfault in git log --notes (also affects diff-tree) when notes.displayRef is configured with no value (not an empty value). It appears to stem from a missing return statement in notes.c:notes_display_config(). Very small fix. (Not sure how much sense it makes to include the test.) Thanks, Nate Nate Avers (2): notes.c: fix a segfault in notes_display_config() t3301: test proper exit response to no-value notes.displayRef. notes.c | 2 +- t/t3301-notes.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) -- 2.27.0 ``` ## nate@roosteregg.cc, 2020-11-23 03:23 Subject: [PATCH 1/2] notes.c: fix a segfault in notes_display_config() Message-ID: <20201123032342.24566-2-nate@roosteregg.cc> URL: https://gitlist.dev/e/20201123032342.24566-2-nate%40roosteregg.cc In-Reply-To: <20201123032342.24566-1-nate@roosteregg.cc> ``` From: Nate Avers If notes.displayRef is configured with no value[1], control should be returned to the caller when notes.c:notes_display_config() checks if 'v' is NULL. Otherwise, both git log --notes and git diff-tree --notes will subsequently segfault when refs.h:has_glob_specials() calls strpbrk() with a NULL first argument. [1] Examples: .git/config: [notes] displayRef $ git -c notes.displayRef [...] Signed-off-by: Nate Avers --- notes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes.c b/notes.c index 564baac64d..d5ac081e76 100644 --- a/notes.c +++ b/notes.c @@ -970,7 +970,7 @@ static int notes_display_config(const char *k, const char *v, void *cb) if (*load_refs && !strcmp(k, "notes.displayref")) { if (!v) - config_error_nonbool(k); + return config_error_nonbool(k); string_list_add_refs_by_glob(&display_notes_refs, v); } -- 2.27.0 ``` ## nate@roosteregg.cc, 2020-11-23 03:23 Subject: [PATCH 2/2] t3301: test proper exit response to no-value notes.displayRef. Message-ID: <20201123032342.24566-3-nate@roosteregg.cc> URL: https://gitlist.dev/e/20201123032342.24566-3-nate%40roosteregg.cc In-Reply-To: <20201123032342.24566-1-nate@roosteregg.cc> ``` From: Nate Avers Signed-off-by: Nate Avers --- t/t3301-notes.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 8f43303007..ca60faf480 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -672,6 +672,11 @@ test_expect_success 'notes.displayRef respects order' ' test_cmp expect-both-reversed actual ' +test_expect_success 'notes.displayRef with no value handled gracefully' ' + test_must_fail git -c notes.displayRef log -0 --notes && + test_must_fail git -c notes.displayRef diff-tree --notes HEAD +' + test_expect_success 'GIT_NOTES_DISPLAY_REF works' ' GIT_NOTES_DISPLAY_REF=refs/notes/commits:refs/notes/other \ git log -2 >actual && -- 2.27.0 ``` ## Junio C Hamano, 2020-11-23 06:59 Subject: Re: [PATCH 1/2] notes.c: fix a segfault in notes_display_config() Message-ID: URL: https://gitlist.dev/e/xmqqzh38o8x8.fsf%40gitster.c.googlers.com In-Reply-To: <20201123032342.24566-2-nate@roosteregg.cc> ``` nate@roosteregg.cc writes: > if (*load_refs && !strcmp(k, "notes.displayref")) { > if (!v) > - config_error_nonbool(k); > + return config_error_nonbool(k); "git grep config_error_nonbool" tells us that this is the only instance that ignores the return value from the function and does not cause the caller to die. Looks good. Thanks. ``` ## Junio C Hamano, 2020-11-23 18:44 Subject: Re: [PATCH 2/2] t3301: test proper exit response to no-value notes.displayRef. Message-ID: URL: https://gitlist.dev/e/xmqqr1ojoqvi.fsf%40gitster.c.googlers.com In-Reply-To: <20201123032342.24566-3-nate@roosteregg.cc> ``` nate@roosteregg.cc writes: > From: Nate Avers > > Signed-off-by: Nate Avers > --- > t/t3301-notes.sh | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh > index 8f43303007..ca60faf480 100755 > --- a/t/t3301-notes.sh > +++ b/t/t3301-notes.sh > @@ -672,6 +672,11 @@ test_expect_success 'notes.displayRef respects order' ' > test_cmp expect-both-reversed actual > ' > > +test_expect_success 'notes.displayRef with no value handled gracefully' ' > + test_must_fail git -c notes.displayRef log -0 --notes && > + test_must_fail git -c notes.displayRef diff-tree --notes HEAD > +' Looks good. I didn't know we can feed the "no assignment means true" configuration from the command line with "git -c" ;-) > test_expect_success 'GIT_NOTES_DISPLAY_REF works' ' > GIT_NOTES_DISPLAY_REF=refs/notes/commits:refs/notes/other \ > git log -2 >actual && ```