On 2020-06-05 at 16:26:02, Junio C Hamano wrote:
> diff --git a/fast-import.c b/fast-import.c
> index 0dfa14dc8c..ed87d6e380 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -150,7 +150,7 @@ struct recent_command {
> char *buf;
> };
>
> -typedef void (*mark_set_inserter_t)(struct mark_set *s, struct object_id *oid, uintmax_t mark);
> +typedef void (*mark_set_inserter_t)(struct mark_set **s, struct object_id *oid, uintmax_t mark);
> typedef void (*each_mark_fn_t)(uintmax_t mark, void *obj, void *cbp);
>
> /* Configured limits on output */
> @@ -534,13 +534,15 @@ static char *pool_strdup(const char *s)
> return r;
> }
>
> -static void insert_mark(struct mark_set *s, uintmax_t idnum, struct object_entry *oe)
> +static void insert_mark(struct mark_set **sp, uintmax_t idnum, struct object_entry *oe)
> {
> + struct mark_set *s = *sp;
> +
> while ((idnum >> s->shift) >= 1024) {
> s = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));
> - s->shift = marks->shift + 10;
> - s->data.sets[0] = marks;
> - marks = s;
> + s->shift = (*sp)->shift + 10;
> + s->data.sets[0] = (*sp);
> + (*sp) = s;
> }
Yeah, this is much better. I'm not sure how I missed converting that
block, but it definitely leaks heavily, and obviously we don't want to
update the actual marks variable in every case, since that defeats the
purpose of the patch.
We don't actually hit this case in any of the tests because we don't
have any tests that have 1024 marks in them. I'm having trouble coming
up with a test even with a large number of marks because I don't think
we produce different behavior here; we just leak a lot of memory.
This does fix the reported issue, as I suspected.
Do you want to write this up into a patch with a commit message, or
should I? If the latter, may I have your sign-off for it?
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Attachment:
signature.asc
Description: PGP signature
