# [PATCH] ls-files: don't try to prune an empty index 9 messages from 2017-07-15 to 2017-07-16. Participants: René Scharfe, Ramsay Jones, Jeff King. Thread: https://gitlist.dev/t/46396 ## René Scharfe, 2017-07-15 20:11 Subject: [PATCH] ls-files: don't try to prune an empty index Message-ID: URL: https://gitlist.dev/e/bbc51fb0-70ce-3b02-274b-31b5dc035e29%40web.de ``` Exit early when asked to prune an index that contains no entries to begin with. This avoids pointer arithmetic on istate->cache, which is possibly NULL in that case. Found with Clang's UBSan. Signed-off-by: Rene Scharfe --- builtin/ls-files.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index b8514a0029..adf572da68 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, int pos; unsigned int first, last; - if (!prefix) + if (!prefix || !istate->cache_nr) return; pos = index_name_pos(istate, prefix, prefixlen); if (pos < 0) -- 2.13.3 ``` ## Ramsay Jones, 2017-07-16 00:28 Subject: Re: [PATCH] ls-files: don't try to prune an empty index Message-ID: <8fdcc21d-ccf9-7594-4d69-dfcad580da32@ramsayjones.plus.com> URL: https://gitlist.dev/e/8fdcc21d-ccf9-7594-4d69-dfcad580da32%40ramsayjones.plus.com In-Reply-To: ``` On 15/07/17 21:11, René Scharfe wrote: > Exit early when asked to prune an index that contains no > entries to begin with. This avoids pointer arithmetic on > istate->cache, which is possibly NULL in that case. > > Found with Clang's UBSan. > > Signed-off-by: Rene Scharfe > --- > builtin/ls-files.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > index b8514a0029..adf572da68 100644 > --- a/builtin/ls-files.c > +++ b/builtin/ls-files.c > @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, > int pos; > unsigned int first, last; > > - if (!prefix) > + if (!prefix || !istate->cache_nr) > return; > pos = index_name_pos(istate, prefix, prefixlen); > if (pos < 0) My patch looked like: - if (!prefix) + if (!prefix || !istate->cache || istate->cache_nr == 0) ... which is probably a bit 'belt-n-braces'. ;-) ATB, Ramsay Jones ``` ## René Scharfe, 2017-07-16 03:52 Subject: Re: [PATCH] ls-files: don't try to prune an empty index Message-ID: URL: https://gitlist.dev/e/ae5359cf-a480-f357-cb51-78f9d4cc431c%40web.de In-Reply-To: <8fdcc21d-ccf9-7594-4d69-dfcad580da32@ramsayjones.plus.com> ``` Am 16.07.2017 um 02:28 schrieb Ramsay Jones: > > > On 15/07/17 21:11, René Scharfe wrote: >> Exit early when asked to prune an index that contains no >> entries to begin with. This avoids pointer arithmetic on >> istate->cache, which is possibly NULL in that case. >> >> Found with Clang's UBSan. >> >> Signed-off-by: Rene Scharfe >> --- >> builtin/ls-files.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/builtin/ls-files.c b/builtin/ls-files.c >> index b8514a0029..adf572da68 100644 >> --- a/builtin/ls-files.c >> +++ b/builtin/ls-files.c >> @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, >> int pos; >> unsigned int first, last; >> >> - if (!prefix) >> + if (!prefix || !istate->cache_nr) >> return; >> pos = index_name_pos(istate, prefix, prefixlen); >> if (pos < 0) > > My patch looked like: > > - if (!prefix) > + if (!prefix || !istate->cache || istate->cache_nr == 0) > > ... which is probably a bit 'belt-n-braces'. ;-) Not checking for !istate->cache at this point is a good thing, I think. If we have entries, then ->cache must not be NULL, and if it is we'd get a segfault, notifying us that we have a bug. We could add an assert to state this requirement explicitly, but that would be the topic of a different patch. René ``` ## Jeff King, 2017-07-16 10:41 Subject: Re: [PATCH] ls-files: don't try to prune an empty index Message-ID: <20170716104114.7tfcuilsy3ab74n6@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170716104114.7tfcuilsy3ab74n6%40sigill.intra.peff.net In-Reply-To: ``` On Sat, Jul 15, 2017 at 10:11:20PM +0200, René Scharfe wrote: > Exit early when asked to prune an index that contains no > entries to begin with. This avoids pointer arithmetic on > istate->cache, which is possibly NULL in that case. > > Found with Clang's UBSan. Makes sense. We could use MOVE_ARRAY() here, but this is a sensible short-circuit to the whole function. Looks like a good solution. > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > index b8514a0029..adf572da68 100644 > --- a/builtin/ls-files.c > +++ b/builtin/ls-files.c > @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, > int pos; > unsigned int first, last; > > - if (!prefix) > + if (!prefix || !istate->cache_nr) > return; > pos = index_name_pos(istate, prefix, prefixlen); > if (pos < 0) "git am" complained that this does not apply to its blobs. Did you hand-edit? -Peff ``` ## René Scharfe, 2017-07-16 11:06 Subject: Re: [PATCH] ls-files: don't try to prune an empty index Message-ID: URL: https://gitlist.dev/e/e91c204d-0bcb-9614-b62d-cd87c03b92e1%40web.de In-Reply-To: <20170716104114.7tfcuilsy3ab74n6@sigill.intra.peff.net> ``` Am 16.07.2017 um 12:41 schrieb Jeff King: > On Sat, Jul 15, 2017 at 10:11:20PM +0200, René Scharfe wrote: > >> Exit early when asked to prune an index that contains no >> entries to begin with. This avoids pointer arithmetic on >> istate->cache, which is possibly NULL in that case. >> >> Found with Clang's UBSan. > > Makes sense. We could use MOVE_ARRAY() here, but this is a sensible > short-circuit to the whole function. > > Looks like a good solution. > >> diff --git a/builtin/ls-files.c b/builtin/ls-files.c >> index b8514a0029..adf572da68 100644 >> --- a/builtin/ls-files.c >> +++ b/builtin/ls-files.c >> @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, >> int pos; >> unsigned int first, last; >> >> - if (!prefix) >> + if (!prefix || !istate->cache_nr) >> return; >> pos = index_name_pos(istate, prefix, prefixlen); >> if (pos < 0) > > "git am" complained that this does not apply to its blobs. Did you > hand-edit? I didn't, but perhaps I messed up the order of patches? MOVE_ARRAY patch 2 touches the same file, but I wouldn't expect the two changes to conflict. So not sure what's going on. René ``` ## Jeff King, 2017-07-16 11:08 Subject: Re: [PATCH] ls-files: don't try to prune an empty index Message-ID: <20170716110831.c442ihan6tzf7gmn@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170716110831.c442ihan6tzf7gmn%40sigill.intra.peff.net In-Reply-To: ``` On Sun, Jul 16, 2017 at 01:06:45PM +0200, René Scharfe wrote: > > > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > > > index b8514a0029..adf572da68 100644 > > > --- a/builtin/ls-files.c > > > +++ b/builtin/ls-files.c > > > @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, > > > int pos; > > > unsigned int first, last; > > > > > > - if (!prefix) > > > + if (!prefix || !istate->cache_nr) > > > return; > > > pos = index_name_pos(istate, prefix, prefixlen); > > > if (pos < 0) > > > > "git am" complained that this does not apply to its blobs. Did you > > hand-edit? > > I didn't, but perhaps I messed up the order of patches? MOVE_ARRAY > patch 2 touches the same file, but I wouldn't expect the two changes to > conflict. So not sure what's going on. For some reason there's an extra space before the tab on each of the context lines. MUA issue or cut-and-paste, maybe? -Peff ``` ## René Scharfe, 2017-07-16 11:16 Subject: [PATCH (resend)] ls-files: don't try to prune an empty index Message-ID: <4522dcd2-a896-4c41-89fd-c3f33a23a3d6@web.de> URL: https://gitlist.dev/e/4522dcd2-a896-4c41-89fd-c3f33a23a3d6%40web.de In-Reply-To: ``` Exit early when asked to prune an index that contains no entries to begin with. This avoids pointer arithmetic on istate->cache, which is possibly NULL in that case. Found with Clang's UBSan. Signed-off-by: Rene Scharfe --- Messed up whitespace when sending this the first time. builtin/ls-files.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index b8514a0029..adf572da68 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, int pos; unsigned int first, last; - if (!prefix) + if (!prefix || !istate->cache_nr) return; pos = index_name_pos(istate, prefix, prefixlen); if (pos < 0) -- 2.13.3 ``` ## René Scharfe, 2017-07-16 11:15 Subject: Re: [PATCH] ls-files: don't try to prune an empty index Message-ID: <31e6bebd-f2f5-b85d-2865-49611aecb2b8@web.de> URL: https://gitlist.dev/e/31e6bebd-f2f5-b85d-2865-49611aecb2b8%40web.de In-Reply-To: <20170716110831.c442ihan6tzf7gmn@sigill.intra.peff.net> ``` Am 16.07.2017 um 13:08 schrieb Jeff King: > On Sun, Jul 16, 2017 at 01:06:45PM +0200, René Scharfe wrote: > >>>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c >>>> index b8514a0029..adf572da68 100644 >>>> --- a/builtin/ls-files.c >>>> +++ b/builtin/ls-files.c >>>> @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, >>>> int pos; >>>> unsigned int first, last; >>>> >>>> - if (!prefix) >>>> + if (!prefix || !istate->cache_nr) >>>> return; >>>> pos = index_name_pos(istate, prefix, prefixlen); >>>> if (pos < 0) >>> >>> "git am" complained that this does not apply to its blobs. Did you >>> hand-edit? >> >> I didn't, but perhaps I messed up the order of patches? MOVE_ARRAY >> patch 2 touches the same file, but I wouldn't expect the two changes to >> conflict. So not sure what's going on. > > For some reason there's an extra space before the tab on each of the > context lines. MUA issue or cut-and-paste, maybe? That's possible. Will resend. ``` ## René Scharfe, 2017-07-16 11:32 Subject: Re: [PATCH] ls-files: don't try to prune an empty index Message-ID: URL: https://gitlist.dev/e/c408be84-c35f-bf3a-4699-9dd0b5de93ef%40web.de In-Reply-To: <31e6bebd-f2f5-b85d-2865-49611aecb2b8@web.de> ``` Am 16.07.2017 um 13:15 schrieb René Scharfe: > Am 16.07.2017 um 13:08 schrieb Jeff King: >> On Sun, Jul 16, 2017 at 01:06:45PM +0200, René Scharfe wrote: >> >>>>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c >>>>> index b8514a0029..adf572da68 100644 >>>>> --- a/builtin/ls-files.c >>>>> +++ b/builtin/ls-files.c >>>>> @@ -362,7 +362,7 @@ static void prune_index(struct index_state >>>>> *istate, >>>>> int pos; >>>>> unsigned int first, last; >>>>> >>>>> - if (!prefix) >>>>> + if (!prefix || !istate->cache_nr) >>>>> return; >>>>> pos = index_name_pos(istate, prefix, prefixlen); >>>>> if (pos < 0) >>>> >>>> "git am" complained that this does not apply to its blobs. Did you >>>> hand-edit? >>> >>> I didn't, but perhaps I messed up the order of patches? MOVE_ARRAY >>> patch 2 touches the same file, but I wouldn't expect the two changes to >>> conflict. So not sure what's going on. >> >> For some reason there's an extra space before the tab on each of the >> context lines. MUA issue or cut-and-paste, maybe? > > That's possible. Will resend. ... pressed Send to fast. Thanks for reporting the broken patch! I use the extension Toggle Word Wrap with Thunderbird, but it wraps by default with no way to change that, so I forgot toggling this time. Grr, I've had enough of this! Went past the warranty warning and set mailnews.wraplength=0 now. René ```