Re: [PATCH v2] sha1_name: fix uninitialized memory errors
Derrick Stolee writes: >> I do not think they are wrong, but aren't the latter two somewhat >> redundant? "num" is p->num_objects, and we call (first+1)th element >> only after we see (first < num - 1), i.e. first+1 < num, and the >> access to (first-1)th is done only when first > 0. The first one, >> i.e. when first points at where we _would_ find it if it existed, >> can access "first" that could be p->num_objects, so the change there >> makes sense, though. > > Yes. But I'd rather keep the blocks consistent and use the return > value of nth_packed_object_oid() when possible. Sure, I do not think anybody minds; I just wanted a sanity check. Thansk.
Re: [PATCH v2] sha1_name: fix uninitialized memory errors
On 2/28/2018 3:50 PM, Junio C Hamano wrote:
Derrick Stolee writes:
diff --git a/sha1_name.c b/sha1_name.c
index 611c7d24dd..a041d8d24f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
*/
mad->init_len = 0;
if (!match) {
- nth_packed_object_oid(&oid, p, first);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first))
+ extend_abbrev_len(&oid, mad);
} else if (first < num - 1) {
- nth_packed_object_oid(&oid, p, first + 1);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first + 1))
+ extend_abbrev_len(&oid, mad);
}
if (first > 0) {
- nth_packed_object_oid(&oid, p, first - 1);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first - 1))
+ extend_abbrev_len(&oid, mad);
}
mad->init_len = mad->cur_len;
}
I do not think they are wrong, but aren't the latter two somewhat
redundant? "num" is p->num_objects, and we call (first+1)th element
only after we see (first < num - 1), i.e. first+1 < num, and the
access to (first-1)th is done only when first > 0. The first one,
i.e. when first points at where we _would_ find it if it existed,
can access "first" that could be p->num_objects, so the change there
makes sense, though.
Yes. But I'd rather keep the blocks consistent and use the return value
of nth_packed_object_oid() when possible.
Thanks,
-Stolee
Re: [PATCH v2] sha1_name: fix uninitialized memory errors
Derrick Stolee writes:
> diff --git a/sha1_name.c b/sha1_name.c
> index 611c7d24dd..a041d8d24f 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git
> *p,
>*/
> mad->init_len = 0;
> if (!match) {
> - nth_packed_object_oid(&oid, p, first);
> - extend_abbrev_len(&oid, mad);
> + if (nth_packed_object_oid(&oid, p, first))
> + extend_abbrev_len(&oid, mad);
> } else if (first < num - 1) {
> - nth_packed_object_oid(&oid, p, first + 1);
> - extend_abbrev_len(&oid, mad);
> + if (nth_packed_object_oid(&oid, p, first + 1))
> + extend_abbrev_len(&oid, mad);
> }
> if (first > 0) {
> - nth_packed_object_oid(&oid, p, first - 1);
> - extend_abbrev_len(&oid, mad);
> + if (nth_packed_object_oid(&oid, p, first - 1))
> + extend_abbrev_len(&oid, mad);
> }
> mad->init_len = mad->cur_len;
> }
I do not think they are wrong, but aren't the latter two somewhat
redundant? "num" is p->num_objects, and we call (first+1)th element
only after we see (first < num - 1), i.e. first+1 < num, and the
access to (first-1)th is done only when first > 0. The first one,
i.e. when first points at where we _would_ find it if it existed,
can access "first" that could be p->num_objects, so the change there
makes sense, though.
Re: [PATCH v2] sha1_name: fix uninitialized memory errors
On Tue, Feb 27, 2018 at 02:30:39PM -0800, Junio C Hamano wrote: > -- >8 -- > From: Derrick Stolee > Date: Tue, 27 Feb 2018 06:47:04 -0500 > Subject: [PATCH] sha1_name: fix uninitialized memory errors > > 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. > > Also the comment about checking near-by objects miscounts the > neighbours. If we have a hit at "first", we check "first-1" and > "first+1" to make sure we have sufficiently long abbreviation not to > match either. If we do not have a hit, "first" is the smallest > among the objects that are larger than what we want to name, so we > check that and "first-1" to make sure we have sufficiently long > abbreviation not to match either. In either case, we only check up > to two near-by objects. Yep, this looks good to me. Thanks for wrapping it up. -Peff
Re: [PATCH v2] sha1_name: fix uninitialized memory errors
Jeff King writes:
> Thanks, this looks good to me.
>
> Semi-related, I wondered also at the weird asymmetry in the if-else,
> ...
> So I think the code is right, but the comment is wrong.
-- >8 --
From: Derrick Stolee
Date: Tue, 27 Feb 2018 06:47:04 -0500
Subject: [PATCH] sha1_name: fix uninitialized memory errors
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.
Also the comment about checking near-by objects miscounts the
neighbours. If we have a hit at "first", we check "first-1" and
"first+1" to make sure we have sufficiently long abbreviation not to
match either. If we do not have a hit, "first" is the smallest
among the objects that are larger than what we want to name, so we
check that and "first-1" to make sure we have sufficiently long
abbreviation not to match either. In either case, we only check up
to two near-by objects.
Reported-by: Christian Couder
Signed-off-by: Derrick Stolee
Signed-off-by: Junio C Hamano
---
sha1_name.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 05a635911b..f1c3d37a6d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -542,20 +542,20 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
/*
* first is now the position in the packfile where we would insert
* mad->hash if it does not exist (or the position of mad->hash if
-* it does exist). Hence, we consider a maximum of three objects
+* it does exist). Hence, we consider a maximum of two objects
* nearby for the abbreviation length.
*/
mad->init_len = 0;
if (!match) {
- nth_packed_object_oid(&oid, p, first);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first))
+ extend_abbrev_len(&oid, mad);
} else if (first < num - 1) {
- nth_packed_object_oid(&oid, p, first + 1);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first + 1))
+ extend_abbrev_len(&oid, mad);
}
if (first > 0) {
- nth_packed_object_oid(&oid, p, first - 1);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first - 1))
+ extend_abbrev_len(&oid, mad);
}
mad->init_len = mad->cur_len;
}
--
2.16.2-325-g2fc74f41c5
Re: [PATCH v2] sha1_name: fix uninitialized memory errors
On Tue, Feb 27, 2018 at 06:47:04AM -0500, Derrick Stolee wrote: > Peff made an excellent point about the nested if statements. This > goes back to Christian's original recommendation. > > -- >8 -- > > 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 Thanks, this looks good to me. Semi-related, I wondered also at the weird asymmetry in the if-else, which is: if ... else if ... if ... but the comment directly above says: "we consider a maximum of three objects nearby". I think it's actually two, because you can only trigger one of the first two conditionals. Is that right? Let's imagine we're looking for object 1234abcd. If we didn't find a match, then we might have: 1234abcc 1234abce <-- first points here in which case we need to check both first-1 and first. And we do. If we do have a match, then we might see: 1234abcc 1234abcd <-- first points here 1234abce and we must check first-1 and first+1, but _not_ first. So I think the code is right, but the comment is wrong. -Peff
[PATCH v2] sha1_name: fix uninitialized memory errors
Peff made an excellent point about the nested if statements. This
goes back to Christian's original recommendation.
-- >8 --
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 | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 611c7d24dd..a041d8d24f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
*/
mad->init_len = 0;
if (!match) {
- nth_packed_object_oid(&oid, p, first);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first))
+ extend_abbrev_len(&oid, mad);
} else if (first < num - 1) {
- nth_packed_object_oid(&oid, p, first + 1);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first + 1))
+ extend_abbrev_len(&oid, mad);
}
if (first > 0) {
- nth_packed_object_oid(&oid, p, first - 1);
- extend_abbrev_len(&oid, mad);
+ if (nth_packed_object_oid(&oid, p, first - 1))
+ extend_abbrev_len(&oid, mad);
}
mad->init_len = mad->cur_len;
}
--
2.16.2.265.g3d5930c0b9.dirty
