Re: [PATCH] sha1_name: fix uninitialized memory errors

[Jeff King]

On Mon, Feb 26, 2018 at 09:56:47AM -0500, Derrick Stolee wrote:

> diff --git a/sha1_name.c b/sha1_name.c
> index 611c7d2..44dd595 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -546,17 +546,12 @@ static void find_abbrev_len_for_pack(struct packed_git 
> *p,
>* nearby for the abbreviation length.
>*/
>   mad->init_len = 0;
> - if (!match) {
> - nth_packed_object_oid(, p, first);
> + if (!match && nth_packed_object_oid(, p, first))
>   extend_abbrev_len(, mad);
> - } else if (first < num - 1) {
> - nth_packed_object_oid(, p, first + 1);
> + else if (first < num - 1 && nth_packed_object_oid(, p, first + 1))
>   extend_abbrev_len(, mad);
> - }

I think including the nth_packed_object_oid() in the main if-else chain
works out, but it's kind of tricky.

In the code before, we'd hit the "first < num - 1" conditional only when
we didn't match something. But now we also hit it if we _did_ match
something, but nth_packed_object_oid() didn't work.

But this works out the same if we assume any match must also succeed at
nth_packed_object_oid(). Which in turn implies that checking the result
of nth_packed_object_oid() in the "else if" is redundant (though we
already clamp it to "num - 1", so we'd expect it to always succeed
anyway).

So I think this behaves well, but I wonder if the two-level conditionals
like:

  if (!match) {
if (nth_packed_object_oid(, p, first))
extend_abbrev_len(, mad);
  } else if ...

are easier to reason about.

-Peff

[PATCH] sha1_name: fix uninitialized memory errors

[Derrick Stolee]

During abbreviation checks, we navigate to the position within a
pack-index that an OID would be inserted and check surrounding OIDs
for the maximum matching prefix. This position may be beyond the
last position, because the given OID is lexicographically larger
than every OID in the pack. Then nth_packed_object_oid() does not
initialize "oid".

Use the return value of nth_packed_object_oid() to prevent these
errors.

Reported-by: Christian Couder 
Signed-off-by: Derrick Stolee 
---
 sha1_name.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 611c7d2..44dd595 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -546,17 +546,12 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
 * nearby for the abbreviation length.
 */
mad->init_len = 0;
-   if (!match) {
-   nth_packed_object_oid(, p, first);
+   if (!match && nth_packed_object_oid(, p, first))
extend_abbrev_len(, mad);
-   } else if (first < num - 1) {
-   nth_packed_object_oid(, p, first + 1);
+   else if (first < num - 1 && nth_packed_object_oid(, p, first + 1))
extend_abbrev_len(, mad);
-   }
-   if (first > 0) {
-   nth_packed_object_oid(, p, first - 1);
+   if (first > 0 && nth_packed_object_oid(, p, first - 1))
extend_abbrev_len(, mad);
-   }
mad->init_len = mad->cur_len;
 }
 
-- 
2.7.4