* [PATCH] ls-files: don't try to prune an empty index
@ 2017-07-15 20:11 René Scharfe
2017-07-16 0:28 ` Ramsay Jones
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: René Scharfe @ 2017-07-15 20:11 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jeff King
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 <l.s.r@web.de>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] ls-files: don't try to prune an empty index
2017-07-15 20:11 [PATCH] ls-files: don't try to prune an empty index René Scharfe
@ 2017-07-16 0:28 ` Ramsay Jones
2017-07-16 3:52 ` René Scharfe
2017-07-16 10:41 ` Jeff King
2017-07-16 11:16 ` [PATCH (resend)] " René Scharfe
2 siblings, 1 reply; 9+ messages in thread
From: Ramsay Jones @ 2017-07-16 0:28 UTC (permalink / raw)
To: René Scharfe, Git List; +Cc: Junio C Hamano, Jeff King
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 <l.s.r@web.de>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ls-files: don't try to prune an empty index
2017-07-16 0:28 ` Ramsay Jones
@ 2017-07-16 3:52 ` René Scharfe
0 siblings, 0 replies; 9+ messages in thread
From: René Scharfe @ 2017-07-16 3:52 UTC (permalink / raw)
To: Ramsay Jones, Git List; +Cc: Junio C Hamano, Jeff King
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 <l.s.r@web.de>
>> ---
>> 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é
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ls-files: don't try to prune an empty index
2017-07-15 20:11 [PATCH] ls-files: don't try to prune an empty index René Scharfe
2017-07-16 0:28 ` Ramsay Jones
@ 2017-07-16 10:41 ` Jeff King
2017-07-16 11:06 ` René Scharfe
2017-07-16 11:16 ` [PATCH (resend)] " René Scharfe
2 siblings, 1 reply; 9+ messages in thread
From: Jeff King @ 2017-07-16 10:41 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ls-files: don't try to prune an empty index
2017-07-16 10:41 ` Jeff King
@ 2017-07-16 11:06 ` René Scharfe
2017-07-16 11:08 ` Jeff King
0 siblings, 1 reply; 9+ messages in thread
From: René Scharfe @ 2017-07-16 11:06 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
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é
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ls-files: don't try to prune an empty index
2017-07-16 11:06 ` René Scharfe
@ 2017-07-16 11:08 ` Jeff King
2017-07-16 11:15 ` René Scharfe
0 siblings, 1 reply; 9+ messages in thread
From: Jeff King @ 2017-07-16 11:08 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ls-files: don't try to prune an empty index
2017-07-16 11:08 ` Jeff King
@ 2017-07-16 11:15 ` René Scharfe
2017-07-16 11:32 ` René Scharfe
0 siblings, 1 reply; 9+ messages in thread
From: René Scharfe @ 2017-07-16 11:15 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ls-files: don't try to prune an empty index
2017-07-16 11:15 ` René Scharfe
@ 2017-07-16 11:32 ` René Scharfe
0 siblings, 0 replies; 9+ messages in thread
From: René Scharfe @ 2017-07-16 11:32 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
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é
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH (resend)] ls-files: don't try to prune an empty index
2017-07-15 20:11 [PATCH] ls-files: don't try to prune an empty index René Scharfe
2017-07-16 0:28 ` Ramsay Jones
2017-07-16 10:41 ` Jeff King
@ 2017-07-16 11:16 ` René Scharfe
2 siblings, 0 replies; 9+ messages in thread
From: René Scharfe @ 2017-07-16 11:16 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jeff King
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 <l.s.r@web.de>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-16 11:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-15 20:11 [PATCH] ls-files: don't try to prune an empty index René Scharfe
2017-07-16 0:28 ` Ramsay Jones
2017-07-16 3:52 ` René Scharfe
2017-07-16 10:41 ` Jeff King
2017-07-16 11:06 ` René Scharfe
2017-07-16 11:08 ` Jeff King
2017-07-16 11:15 ` René Scharfe
2017-07-16 11:32 ` René Scharfe
2017-07-16 11:16 ` [PATCH (resend)] " René Scharfe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).