Re: [PATCH v2 4/6] object_array: use `object_array_clear()`, not `free()`

[Jeff King <peff@peff.net>]

On Sat, Sep 23, 2017 at 01:34:52AM +0200, Martin Ågren wrote:

> Instead of freeing `foo.objects` for an object array `foo` (sometimes
> conditionally), call `object_array_clear(&foo)`. This means we don't
> poke as much into the implementation, which is already a good thing, but
> also that we release the individual entries as well, thereby fixing at
> least one memory-leak (in diff-lib.c).
> 
> If someone is holding on to a pointer to an element's `name` or `path`,
> that is now a dangling pointer, i.e., we'd be turning an unpleasant
> situation into an outright bug. To the best of my understanding no such
> long-term pointers are being taken.

It would be nice if we had a good way to verify this automatically, but
I couldn't think of one. It passes the test suite with ASan, but of
course our coverage is not 100%.

We do know a few things:

  1. Any caller which keeps a pointer to the object-entries themselves
     is already broken, since we free that already. We only have to care
     about the name and path strings.

  2. Any caller which uses add_object_array() has a NULL path, so we
     don't have to worry about leaving that dangling.

  3. Most of the callers outside of revision.c use a NULL name argument,
     as well.

So let's see:

> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index e237d927a..6b34f23e7 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -182,8 +182,8 @@ static int commit_is_complete(struct commit *commit)
>  			found.objects[i].item->flags |= SEEN;
>  	}
>  	/* free object arrays */
> -	free(study.objects);
> -	free(found.objects);
> +	object_array_clear(&study);
> +	object_array_clear(&found);
>  	return !is_incomplete;
>  }

These ones always have NULL names and paths, so are good.

(An orthogonal low-hanging fruit: these probably ought to use
OBJECT_ARRAY_INIT instead of memset. Maybe a good #leftoverbits for some
of the Outreachy applicants).

> diff --git a/diff-lib.c b/diff-lib.c
> index 2a52b0795..4e0980caa 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -549,7 +549,6 @@ int index_differs_from(const char *def, int diff_flags,
>  	rev.diffopt.flags |= diff_flags;
>  	rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
>  	run_diff_index(&rev, 1);
> -	if (rev.pending.alloc)
> -		free(rev.pending.objects);
> +	object_array_clear(&rev.pending);
>  	return (DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0);
>  }

This one gets the entries from setup_revisions(), so they may have
actual names. It calls diff_flush() before the clear, though, so I'm
pretty sure we would have dropped any queued entries (and I'm also
pretty sure that the queued entries make their own copies of any names
anyway). So this one isn't trivial, but I think it's fine.

> diff --git a/submodule.c b/submodule.c
> index 36f45f5a5..79fd01f7b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1728,7 +1728,7 @@ static int find_first_merges(struct object_array *result, const char *path,
>  			add_object_array(merges.objects[i].item, NULL, result);
>  	}
>  
> -	free(merges.objects);
> +	object_array_clear(&merges);
>  	return result->nr;
>  }

Another always-NULL case.

> @@ -1833,7 +1833,7 @@ int merge_submodule(struct object_id *result, const char *path,
>  			print_commit((struct commit *) merges.objects[i].item);
>  	}
>  
> -	free(merges.objects);
> +	object_array_clear(&merges);
>  	return 0;
>  }

This one is fed by the "result" array of find_first_merges(), which is
also always-NULL.

> diff --git a/upload-pack.c b/upload-pack.c
> index 7efff2fbf..ec0eee8fc 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -888,7 +888,7 @@ static void receive_needs(void)
>  		}
>  
>  	shallow_nr += shallows.nr;
> -	free(shallows.objects);
> +	object_array_clear(&shallows);
>  }

Also always NULL.

So I think all of these cases are good (and weren't actually leaking in
the first place, since the only thing to get rid of was the entries
themselves).

> The way we handle `study` in builting/reflog.c still looks like it might
> leak. That will be addressed in the next commit.

This confused me for a minute, since the leak is not visible in the
context. ;) But reading the next commit, it makes sense.

-Peff