diff mbox series

[01/10] unpack-trees: avoid array out-of-bounds error

Message ID 5bfe3f3fc8a99b3d4fdd4286da17cd935090c614.1588857462.git.gitgitgadget@gmail.com
State New, archived
Headers show
Series In-tree sparse-checkout definitions | expand

Commit Message

Johannes Schindelin via GitGitGadget May 7, 2020, 1:17 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

The loop in warn_conflicted_path() that checks for the count of entries
with the same path uses "i+count" for the array entry. However, the loop
only verifies that the value of count is below the array size. Fix this
by adding i to the condition.

I hit this condition during a test of the in-tree sparse-checkout
feature, so it is exercised by the end of the series.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 unpack-trees.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Junio C Hamano May 7, 2020, 10:27 p.m. UTC | #1
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The loop in warn_conflicted_path() that checks for the count of entries
> with the same path uses "i+count" for the array entry. However, the loop
> only verifies that the value of count is below the array size. Fix this
> by adding i to the condition.
>
> I hit this condition during a test of the in-tree sparse-checkout
> feature, so it is exercised by the end of the series.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  unpack-trees.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 9a3ccd9d083..4f880f2da90 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate,
>  	add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
>  
>  	/* Find out how many higher stage entries at same path */
> -	while (++count < istate->cache_nr &&
> +	while (i + ++count < istate->cache_nr &&
>  	       !strcmp(conflicting_path,
>  		       istate->cache[i+count]->name))
>  		/* do nothing */;

Eek.  Yes, it is obvious that the original is wrong once you point
it out.  But "i + ++count" looks like a line noise, and funny way
that lines are wrapped in the original does not help X-<.

We may want to fix the style and the grammar while we are at it,
perhaps like the attached.

In any case, thanks for a fix.

 unpack-trees.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 6bbf58d28e..c38938d96c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -562,11 +562,11 @@ static int warn_conflicted_path(struct index_state *istate,
 
 	add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
 
-	/* Find out how many higher stage entries at same path */
-	while (++count < istate->cache_nr &&
-	       !strcmp(conflicting_path,
-		       istate->cache[i+count]->name))
-		/* do nothing */;
+	/* Find out how many higher stage entries are at same path */
+	while ((++count) + i < istate->cache_nr &&
+	       !strcmp(conflicting_path, istate->cache[count + i]->name))
+		; /* do nothing */
+
 	return count;
 }
Derrick Stolee May 8, 2020, 12:19 p.m. UTC | #2
On 5/7/2020 6:27 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 9a3ccd9d083..4f880f2da90 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate,
>>  	add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
>>  
>>  	/* Find out how many higher stage entries at same path */
>> -	while (++count < istate->cache_nr &&
>> +	while (i + ++count < istate->cache_nr &&
>>  	       !strcmp(conflicting_path,
>>  		       istate->cache[i+count]->name))
>>  		/* do nothing */;
> 
> Eek.  Yes, it is obvious that the original is wrong once you point
> it out.  But "i + ++count" looks like a line noise, and funny way
> that lines are wrapped in the original does not help X-<.
> 
> We may want to fix the style and the grammar while we are at it,
> perhaps like the attached.
> 
> In any case, thanks for a fix.
> 
>  unpack-trees.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 6bbf58d28e..c38938d96c 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -562,11 +562,11 @@ static int warn_conflicted_path(struct index_state *istate,
>  
>  	add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
>  
> -	/* Find out how many higher stage entries at same path */
> -	while (++count < istate->cache_nr &&
> -	       !strcmp(conflicting_path,
> -		       istate->cache[i+count]->name))
> -		/* do nothing */;
> +	/* Find out how many higher stage entries are at same path */
> +	while ((++count) + i < istate->cache_nr &&
> +	       !strcmp(conflicting_path, istate->cache[count + i]->name))
> +		; /* do nothing */
> +

This looks much better, thanks!

As I mentioned in the cover letter, this is worth taking on its own. Could
you queue the collaborative patch? I'll eject it from the next version of
this series. 

Thanks,
-Stolee
Junio C Hamano May 8, 2020, 3:09 p.m. UTC | #3
Derrick Stolee <stolee@gmail.com> writes:

> As I mentioned in the cover letter, this is worth taking on its own. Could
> you queue the collaborative patch? I'll eject it from the next version of
> this series. 

Yes, I think this is worth taking independently.

I do not think people would find the updated version "much better",
but it made it readable at least to me.

Thanks.
Elijah Newren May 20, 2020, 4:32 p.m. UTC | #4
On Thu, May 7, 2020 at 3:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Derrick Stolee <dstolee@microsoft.com>
> >
> > The loop in warn_conflicted_path() that checks for the count of entries
> > with the same path uses "i+count" for the array entry. However, the loop
> > only verifies that the value of count is below the array size. Fix this
> > by adding i to the condition.
> >
> > I hit this condition during a test of the in-tree sparse-checkout
> > feature, so it is exercised by the end of the series.
> >
> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> > ---
> >  unpack-trees.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/unpack-trees.c b/unpack-trees.c
> > index 9a3ccd9d083..4f880f2da90 100644
> > --- a/unpack-trees.c
> > +++ b/unpack-trees.c
> > @@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate,
> >       add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
> >
> >       /* Find out how many higher stage entries at same path */
> > -     while (++count < istate->cache_nr &&
> > +     while (i + ++count < istate->cache_nr &&
> >              !strcmp(conflicting_path,
> >                      istate->cache[i+count]->name))
> >               /* do nothing */;
>
> Eek.  Yes, it is obvious that the original is wrong once you point
> it out.  But "i + ++count" looks like a line noise, and funny way
> that lines are wrapped in the original does not help X-<.

Eek, indeed.  :-(

> We may want to fix the style and the grammar while we are at it,
> perhaps like the attached.
>
> In any case, thanks for a fix.
>
>  unpack-trees.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 6bbf58d28e..c38938d96c 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -562,11 +562,11 @@ static int warn_conflicted_path(struct index_state *istate,
>
>         add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
>
> -       /* Find out how many higher stage entries at same path */
> -       while (++count < istate->cache_nr &&
> -              !strcmp(conflicting_path,
> -                      istate->cache[i+count]->name))
> -               /* do nothing */;
> +       /* Find out how many higher stage entries are at same path */
> +       while ((++count) + i < istate->cache_nr &&
> +              !strcmp(conflicting_path, istate->cache[count + i]->name))
> +               ; /* do nothing */
> +
>         return count;
>  }

Thanks, both.
diff mbox series

Patch

diff --git a/unpack-trees.c b/unpack-trees.c
index 9a3ccd9d083..4f880f2da90 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -563,10 +563,11 @@  static int warn_conflicted_path(struct index_state *istate,
 	add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
 
 	/* Find out how many higher stage entries at same path */
-	while (++count < istate->cache_nr &&
+	while (i + ++count < istate->cache_nr &&
 	       !strcmp(conflicting_path,
 		       istate->cache[i+count]->name))
 		/* do nothing */;
+
 	return count;
 }