Re: [PATCH v2 1/1] commit-reach: properly peel tags
Derrick Stolee writes:
>> +if (!from_one || from_one->type != OBJ_COMMIT) {
>> +/* no way to tell if this is reachable by
>> + * looking at the ancestry chain alone, so
>> + * leave a note to ourselves not to worry about
>> + * this object anymore.
>> + */
>> +from->objects[i].item->flags |= assign_flag;
>> +continue;
>> +}
>> +
>> +list[nr_commits] = (struct commit *)from_one;
>> +if (parse_commit(list[nr_commits]) ||
>> +list[nr_commits]->generation < min_generation)
>> +return 0; /* is this a leak? */
>
> Of course, after sending v2, I see this comment. This is a leak of
> 'list' and should be fixed.
>
> Not only is it a leak here, it is also a leak in the 'cleanup'
> section. I'll squash the following into v3, but I'll let v2 simmer for
> review before rerolling.
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 4048a2132a..c457d8d85f 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -569,8 +569,11 @@ int can_all_from_reach_with_flag(struct
> object_array *from,
>
> list[nr_commits] = (struct commit *)from_one;
> if (parse_commit(list[nr_commits]) ||
> - list[nr_commits]->generation < min_generation)
> - return 0; /* is this a leak? */
> + list[nr_commits]->generation < min_generation) {
> + result = 0;
> + goto cleanup;
> + }
> +
> nr_commits++;
> }
>
> @@ -623,6 +626,7 @@ int can_all_from_reach_with_flag(struct
> object_array *from,
> clear_commit_marks(list[i], RESULT);
> clear_commit_marks(list[i], assign_flag);
> }
> + free(list);
> return result;
> }
With this, commit marks are cleared even when we do the "early
return", but only for the objects that appear in the resulting
list[]. Because the for() loop in the last hunk interates over
list[], those non-commit objects that are smudged with assign_flag
but never made to list[] will be left smudged when this function
returns (this is true when there is no early return).
Is that intended?
Re: [PATCH v2 1/1] commit-reach: properly peel tags
On 9/13/2018 12:10 PM, Derrick Stolee via GitGitGadget wrote:
From: Derrick Stolee
The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
"commit-reach: make can_all_from_reach... linear" but incorrectly
assumed that all objects provided were commits. During a fetch
negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
to the 'from' array. The current code creates a segfault.
Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
and add a test in t6600-test-reach.sh that demonstrates this segfault.
Correct the issue by peeling tags when investigating the initial list
of objects in the 'from' array.
Signed-off-by: Jeff King
Signed-off-by: Derrick Stolee
---
commit-reach.c| 33 ++---
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 --
3 files changed, 71 insertions(+), 14 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 86715c103c..4048a2132a 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -544,20 +544,39 @@ int can_all_from_reach_with_flag(struct object_array
*from,
{
struct commit **list = NULL;
int i;
+ int nr_commits;
int result = 1;
ALLOC_ARRAY(list, from->nr);
+ nr_commits = 0;
for (i = 0; i < from->nr; i++) {
- list[i] = (struct commit *)from->objects[i].item;
+ struct object *from_one = from->objects[i].item;
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
+ if (!from_one || from_one->flags & assign_flag)
+ continue;
+
+ from_one = deref_tag(the_repository, from_one,
+"a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ /* no way to tell if this is reachable by
+* looking at the ancestry chain alone, so
+* leave a note to ourselves not to worry about
+* this object anymore.
+*/
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
+ list[nr_commits]->generation < min_generation)
+ return 0; /* is this a leak? */
Of course, after sending v2, I see this comment. This is a leak of
'list' and should be fixed.
Not only is it a leak here, it is also a leak in the 'cleanup' section.
I'll squash the following into v3, but I'll let v2 simmer for review
before rerolling.
diff --git a/commit-reach.c b/commit-reach.c
index 4048a2132a..c457d8d85f 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -569,8 +569,11 @@ int can_all_from_reach_with_flag(struct
object_array *from,
list[nr_commits] = (struct commit *)from_one;
if (parse_commit(list[nr_commits]) ||
- list[nr_commits]->generation < min_generation)
- return 0; /* is this a leak? */
+ list[nr_commits]->generation < min_generation) {
+ result = 0;
+ goto cleanup;
+ }
+
nr_commits++;
}
@@ -623,6 +626,7 @@ int can_all_from_reach_with_flag(struct object_array
*from,
clear_commit_marks(list[i], RESULT);
clear_commit_marks(list[i], assign_flag);
}
+ free(list);
return result;
}
[PATCH v2 1/1] commit-reach: properly peel tags
From: Derrick Stolee
The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
"commit-reach: make can_all_from_reach... linear" but incorrectly
assumed that all objects provided were commits. During a fetch
negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
to the 'from' array. The current code creates a segfault.
Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
and add a test in t6600-test-reach.sh that demonstrates this segfault.
Correct the issue by peeling tags when investigating the initial list
of objects in the 'from' array.
Signed-off-by: Jeff King
Signed-off-by: Derrick Stolee
---
commit-reach.c| 33 ++---
t/helper/test-reach.c | 22 +-
t/t6600-test-reach.sh | 30 --
3 files changed, 71 insertions(+), 14 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 86715c103c..4048a2132a 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -544,20 +544,39 @@ int can_all_from_reach_with_flag(struct object_array
*from,
{
struct commit **list = NULL;
int i;
+ int nr_commits;
int result = 1;
ALLOC_ARRAY(list, from->nr);
+ nr_commits = 0;
for (i = 0; i < from->nr; i++) {
- list[i] = (struct commit *)from->objects[i].item;
+ struct object *from_one = from->objects[i].item;
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
+ if (!from_one || from_one->flags & assign_flag)
+ continue;
+
+ from_one = deref_tag(the_repository, from_one,
+"a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ /* no way to tell if this is reachable by
+* looking at the ancestry chain alone, so
+* leave a note to ourselves not to worry about
+* this object anymore.
+*/
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
+ list[nr_commits]->generation < min_generation)
+ return 0; /* is this a leak? */
+ nr_commits++;
}
- QSORT(list, from->nr, compare_commits_by_gen);
+ QSORT(list, nr_commits, compare_commits_by_gen);
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
/* DFS from list[i] */
struct commit_list *stack = NULL;
@@ -600,7 +619,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
}
cleanup:
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
clear_commit_marks(list[i], RESULT);
clear_commit_marks(list[i], assign_flag);
}
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index eb21103998..08d2ea68e8 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -31,6 +31,7 @@ int cmd__reach(int ac, const char **av)
struct object_id oid_A, oid_B;
struct commit *A, *B;
struct commit_list *X, *Y;
+ struct object_array X_obj = OBJECT_ARRAY_INIT;
struct commit **X_array;
int X_nr, X_alloc;
struct strbuf buf = STRBUF_INIT;
@@ -49,7 +50,8 @@ int cmd__reach(int ac, const char **av)
while (strbuf_getline(, stdin) != EOF) {
struct object_id oid;
- struct object *o;
+ struct object *orig;
+ struct object *peeled;
struct commit *c;
if (buf.len < 3)
continue;
@@ -57,14 +59,14 @@ int cmd__reach(int ac, const char **av)
if (get_oid_committish(buf.buf + 2, ))
die("failed to resolve %s", buf.buf + 2);
- o = parse_object(r, );
- o = deref_tag_noverify(o);
+ orig = parse_object(r, );
+ peeled = deref_tag_noverify(orig);
- if (!o)
+ if (!peeled)
die("failed to load commit for input %s resulting in
oid %s\n",
buf.buf, oid_to_hex());
- c = object_as_type(r, o, OBJ_COMMIT, 0);
+ c = object_as_type(r, peeled, OBJ_COMMIT, 0);
if (!c)
die("failed to load commit for input %s resulting in
oid %s\n",
@@ -85,6 +87,7 @@ int cmd__reach(int ac, const char **av)
commit_list_insert(c, );
ALLOC_GROW(X_array, X_nr + 1, X_alloc);
X_array[X_nr++] = c;
+
