threads / patch / 46396

patchls-files: don't try to prune an empty index

Subject: [PATCH] ls-files: don't try to prune an empty index

## tl;dr

9 messages between Jul 15, 2017 and Jul 16, 2017. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

René Scharfe· Jul 15, 2017, 20:11 UTC · lore

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(-)

Show changes to builtin/ls-files.c +1 −1

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· Jul 16, 2017, 00:28 UTC · re: René Scharfe · lore

Re: [PATCH] ls-files: don't try to prune an empty index

On 15/07/17 21:11, René Scharfe wrote:

Show 24 quoted lines

> 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

René Scharfe· Jul 16, 2017, 03:52 UTC · re: Ramsay Jones · lore

Re: [PATCH] ls-files: don't try to prune an empty index

Am 16.07.2017 um 02:28 schrieb Ramsay Jones:

Show 34 quoted lines

> 
> 
> 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é

Jeff King· Jul 16, 2017, 10:41 UTC · re: René Scharfe · lore

Re: [PATCH] ls-files: don't try to prune an empty index

On Sat, Jul 15, 2017 at 10:11:20PM +0200, René Scharfe wrote:

Show 5 quoted lines

> 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.

Show 13 quoted lines

> 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· Jul 16, 2017, 11:06 UTC · re: Jeff King · lore

Re: [PATCH] ls-files: don't try to prune an empty index

Am 16.07.2017 um 12:41 schrieb Jeff King:

Show 29 quoted lines

> 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· Jul 16, 2017, 11:08 UTC · re: René Scharfe · lore

Re: [PATCH] ls-files: don't try to prune an empty index

On Sun, Jul 16, 2017 at 01:06:45PM +0200, René Scharfe wrote:

Show 20 quoted lines

> > > 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· Jul 16, 2017, 11:15 UTC · re: Jeff King · lore

Re: [PATCH] ls-files: don't try to prune an empty index

Am 16.07.2017 um 13:08 schrieb Jeff King:

Show 25 quoted lines

> 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· Jul 16, 2017, 11:32 UTC · re: René Scharfe · lore

Re: [PATCH] ls-files: don't try to prune an empty index

Am 16.07.2017 um 13:15 schrieb René Scharfe:

Show 29 quoted lines

> 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é

René Scharfe· Jul 16, 2017, 11:16 UTC · re: René Scharfe · lore

[PATCH (resend)] ls-files: don't try to prune an empty index

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(-)

Show changes to builtin/ls-files.c +1 −1

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