| Message ID | 20200504191324.201663-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: avoid memory leaks | expand |
On 5/4/2020 3:13 PM, Jonathan Tan wrote: > A fuzzer running on the entry point provided by fuzz-commit-graph.c > revealed a memory leak when parse_commit_graph() creates a struct > bloom_filter_settings and then returns early due to error. Fix that > error by always freeing that struct first (if it exists) before > returning early due to error. This fuzzer is an excellent tool that I hope can continue to be extended to other features. Here is a case where it found an error in a feature long after the integration was introduced. > While making that change, I also noticed another possible memory leak - > when the BLOOMDATA chunk is provided but not BLOOMINDEXES. Also fix that > error. Thanks for discovering these issues, and for fixing them! Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Thanks, -Stolee
Jonathan Tan <jonathantanmy@google.com> writes: > A fuzzer running on the entry point provided by fuzz-commit-graph.c > revealed a memory leak when parse_commit_graph() creates a struct > bloom_filter_settings and then returns early due to error. Fix that > error by always freeing that struct first (if it exists) before > returning early due to error. > > While making that change, I also noticed another possible memory leak - > when the BLOOMDATA chunk is provided but not BLOOMINDEXES. Also fix that > error. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Here's a memory leak fix revealed by a fuzzer running at $DAYJOB, and > another one that I noticed while making that fix. > --- Thanks. The patch looks good. Will queue.
On Mon, May 04, 2020 at 04:20:41PM -0400, Derrick Stolee wrote: > On 5/4/2020 3:13 PM, Jonathan Tan wrote: > > A fuzzer running on the entry point provided by fuzz-commit-graph.c > > revealed a memory leak when parse_commit_graph() creates a struct > > bloom_filter_settings and then returns early due to error. Fix that > > error by always freeing that struct first (if it exists) before > > returning early due to error. > > This fuzzer is an excellent tool that I hope can continue to be > extended to other features. Here is a case where it found an error in > a feature long after the integration was introduced. > > > While making that change, I also noticed another possible memory leak - > > when the BLOOMDATA chunk is provided but not BLOOMINDEXES. Also fix that > > error. > > Thanks for discovering these issues, and for fixing them! > > Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Since it already appears to be queued, I'm not sure that my review is adding much. But, this fix looks very good to me, and I am grateful for you running a fuzzer against this code, and finding/fixing bugs in it. Reviewed-by: Taylor Blau <me@ttaylorr.com> > Thanks, > -Stolee Thanks, Taylor
diff --git a/commit-graph.c b/commit-graph.c index 6dc777e2f3..1694f0a691 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -281,8 +281,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size) if (data + graph_size - chunk_lookup < GRAPH_CHUNKLOOKUP_WIDTH) { error(_("commit-graph chunk lookup table entry missing; file may be incomplete")); - free(graph); - return NULL; + goto free_and_return; } chunk_id = get_be32(chunk_lookup + 0); @@ -293,8 +292,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size) if (chunk_offset > graph_size - the_hash_algo->rawsz) { error(_("commit-graph improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32), (uint32_t)chunk_offset); - free(graph); - return NULL; + goto free_and_return; } switch (chunk_id) { @@ -361,8 +359,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size) if (chunk_repeated) { error(_("commit-graph chunk id %08x appears multiple times"), chunk_id); - free(graph); - return NULL; + goto free_and_return; } if (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP) @@ -381,17 +378,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size) /* We need both the bloom chunks to exist together. Else ignore the data */ graph->chunk_bloom_indexes = NULL; graph->chunk_bloom_data = NULL; - graph->bloom_filter_settings = NULL; + FREE_AND_NULL(graph->bloom_filter_settings); } hashcpy(graph->oid.hash, graph->data + graph->data_len - graph->hash_len); - if (verify_commit_graph_lite(graph)) { - free(graph); - return NULL; - } + if (verify_commit_graph_lite(graph)) + goto free_and_return; return graph; + +free_and_return: + free(graph->bloom_filter_settings); + free(graph); + return NULL; } static struct commit_graph *load_commit_graph_one(const char *graph_file,
A fuzzer running on the entry point provided by fuzz-commit-graph.c revealed a memory leak when parse_commit_graph() creates a struct bloom_filter_settings and then returns early due to error. Fix that error by always freeing that struct first (if it exists) before returning early due to error. While making that change, I also noticed another possible memory leak - when the BLOOMDATA chunk is provided but not BLOOMINDEXES. Also fix that error. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Here's a memory leak fix revealed by a fuzzer running at $DAYJOB, and another one that I noticed while making that fix. --- commit-graph.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)