| Message ID | 20221102220142.574890-4-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 586d8b5052f6b98c262c872f54216e39f3d56625 |
| Headers | show |
| Series | line-log: plug some memory leaks | expand |
On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote: > Use diff_free_queue() instead of open-coding it. This shortens the > code and make it less repetitive. > > Note that the second hunk in diff_flush() is interesting, because the > 'free_queue' label separates the loop freeing the queue's filepairs > from free()-ing the queue's internal array. This is somewhat > suspicious, but it was not an issue before: there is only one place > from where we jump to this label with a goto, and that is protected by > an 'if (!q->nr && ...)' condition, i.e. we only skipped the loop > freeing the filepairs when there were no filepairs in the queue to > begin with. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > diff.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/diff.c b/diff.c > index ef94175163..03e6ffb5e4 100644 > --- a/diff.c > +++ b/diff.c > @@ -6337,13 +6337,9 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid > int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only) > { > struct diff_queue_struct *q = &diff_queued_diff; > - int i; > int result = diff_get_patch_id(options, oid, diff_header_only); > > - for (i = 0; i < q->nr; i++) > - diff_free_filepair(q->queue[i]); > - > - free(q->queue); > + diff_free_queue(q); > DIFF_QUEUE_CLEAR(q); So, this all looks fine to me. But I did a quick grep around for DIFF_QUEUE_CLEAR(), and this macro is used in quite a few places. Mostly, as far as I can tell, to "empty" out the diff-queue by setting its 'queue' pointer to NULL, and its 'nr' back to 0. Should we be freeing the memory held by the queue there more aggressively? I.e., should we make sure that there is a diff_free_queue() call above each expansion of the DIFF_QUEUE_CLEAR() macro? Thanks, Taylor
On Wed, Nov 02, 2022 at 08:24:09PM -0400, Taylor Blau wrote: > On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote: > > int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only) > > { > > struct diff_queue_struct *q = &diff_queued_diff; > > - int i; > > int result = diff_get_patch_id(options, oid, diff_header_only); > > > > - for (i = 0; i < q->nr; i++) > > - diff_free_filepair(q->queue[i]); > > - > > - free(q->queue); > > + diff_free_queue(q); > > DIFF_QUEUE_CLEAR(q); > > So, this all looks fine to me. But I did a quick grep around for > DIFF_QUEUE_CLEAR(), and this macro is used in quite a few places. > Mostly, as far as I can tell, to "empty" out the diff-queue by setting > its 'queue' pointer to NULL, and its 'nr' back to 0. > > Should we be freeing the memory held by the queue there more > aggressively? I.e., should we make sure that there is a > diff_free_queue() call above each expansion of the DIFF_QUEUE_CLEAR() > macro? Definitely not. DIFF_QUEUE_CLEAR is often used to initialize a just created 'struct diff_queue_struct' instance; by adding a diff_free_queue() above those it would operate on uninitialized memory.
On Mon, Nov 07, 2022 at 05:13:11PM +0100, SZEDER Gábor wrote: > On Wed, Nov 02, 2022 at 08:24:09PM -0400, Taylor Blau wrote: > > On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote: > > > int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only) > > > { > > > struct diff_queue_struct *q = &diff_queued_diff; > > > - int i; > > > int result = diff_get_patch_id(options, oid, diff_header_only); > > > > > > - for (i = 0; i < q->nr; i++) > > > - diff_free_filepair(q->queue[i]); > > > - > > > - free(q->queue); > > > + diff_free_queue(q); > > > DIFF_QUEUE_CLEAR(q); > > > > So, this all looks fine to me. But I did a quick grep around for > > DIFF_QUEUE_CLEAR(), and this macro is used in quite a few places. > > Mostly, as far as I can tell, to "empty" out the diff-queue by setting > > its 'queue' pointer to NULL, and its 'nr' back to 0. > > > > Should we be freeing the memory held by the queue there more > > aggressively? I.e., should we make sure that there is a > > diff_free_queue() call above each expansion of the DIFF_QUEUE_CLEAR() > > macro? > > Definitely not. DIFF_QUEUE_CLEAR is often used to initialize a just > created 'struct diff_queue_struct' instance; by adding a > diff_free_queue() above those it would operate on uninitialized > memory. Thanks for pointing it out. Thanks, Taylor
diff --git a/diff.c b/diff.c index ef94175163..03e6ffb5e4 100644 --- a/diff.c +++ b/diff.c @@ -6337,13 +6337,9 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only) { struct diff_queue_struct *q = &diff_queued_diff; - int i; int result = diff_get_patch_id(options, oid, diff_header_only); - for (i = 0; i < q->nr; i++) - diff_free_filepair(q->queue[i]); - - free(q->queue); + diff_free_queue(q); DIFF_QUEUE_CLEAR(q); return result; @@ -6612,10 +6608,8 @@ void diff_flush(struct diff_options *options) if (output_format & DIFF_FORMAT_CALLBACK) options->format_callback(q, options, options->format_callback_data); - for (i = 0; i < q->nr; i++) - diff_free_filepair(q->queue[i]); free_queue: - free(q->queue); + diff_free_queue(q); DIFF_QUEUE_CLEAR(q); diff_free(options);
Use diff_free_queue() instead of open-coding it. This shortens the code and make it less repetitive. Note that the second hunk in diff_flush() is interesting, because the 'free_queue' label separates the loop freeing the queue's filepairs from free()-ing the queue's internal array. This is somewhat suspicious, but it was not an issue before: there is only one place from where we jump to this label with a goto, and that is protected by an 'if (!q->nr && ...)' condition, i.e. we only skipped the loop freeing the filepairs when there were no filepairs in the queue to begin with. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- diff.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)