| Message ID | 20240826083052.1542228-1-toon@iotcl.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bundle-uri: plug leak in unbundle_from_file() | expand |
On Mon, Aug 26, 2024 at 10:30:52AM +0200, Toon Claes wrote: > When the function returns early, the variable bundle_ref is not released > through strbuf_release(). > > Fix this leak. And while at it, remove assignments in the conditions of > the "if" statements as suggested in the CodingGuidelines. > > Signed-off-by: Toon Claes <toon@iotcl.com> > --- > bundle-uri.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/bundle-uri.c b/bundle-uri.c > index 1e0ee156ba..eb49cba182 100644 > --- a/bundle-uri.c > +++ b/bundle-uri.c > @@ -367,17 +367,21 @@ static int unbundle_from_file(struct repository *r, const char *file) > struct strbuf bundle_ref = STRBUF_INIT; > size_t bundle_prefix_len; > > - if ((bundle_fd = read_bundle_header(file, &header)) < 0) > - return 1; > + bundle_fd = read_bundle_header(file, &header); > + if (bundle_fd < 0) { > + result = 1; > + goto cleanup; > + } > > /* > * Skip the reachability walk here, since we will be adding > * a reachable ref pointing to the new tips, which will reach > * the prerequisite commits. > */ > - if ((result = unbundle(r, &header, bundle_fd, NULL, > - VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)))) > - return 1; > + result = unbundle(r, &header, bundle_fd, NULL, > + VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)); > + if (result) > + goto cleanup; This changes the returned error code from `1` to whatever `unbundle()` returns. Is this intentional? If so, the commit message should explain why this change is safe. Other than that this looks good to me, and the fix does not conflict with any of my leak-plugging series. Thanks! Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Mon, Aug 26, 2024 at 10:30:52AM +0200, Toon Claes wrote: >> When the function returns early, the variable bundle_ref is not released >> through strbuf_release(). >> >> Fix this leak. And while at it, remove assignments in the conditions of >> the "if" statements as suggested in the CodingGuidelines. > ... >> - if ((result = unbundle(r, &header, bundle_fd, NULL, >> - VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)))) >> - return 1; >> + result = unbundle(r, &header, bundle_fd, NULL, >> + VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)); >> + if (result) >> + goto cleanup; > > This changes the returned error code from `1` to whatever `unbundle()` > returns. Is this intentional? If so, the commit message should explain > why this change is safe. Thanks for reviewing carefully. Both of two callers of unbundle_from_file() are used as the condition of an if() statement, so unbundle() that signals an error with -1 wouldn't be a problem, I would think. It may not be a bad idea as a #leftoverbits item, after the dust settles, to clean up the calling convention in this file (may not be limited to the code path that reaches this function) to follow the usual "signal success with 0, failures are signalled with a negative value". Then we can just return the value we got from a failing read_bundle_header(), just the same way we return the value we got from a failing unbundle(). > Other than that this looks good to me, and the fix does not conflict > with any of my leak-plugging series. Yup. Thanks, both.
Junio C Hamano <gitster@pobox.com> writes: > Thanks for reviewing carefully. > > Both of two callers of unbundle_from_file() are used as the > condition of an if() statement, so unbundle() that signals an error > with -1 wouldn't be a problem, I would think. Hi Junio, I've noticed this patch wasn't picked up yet. Is there anything you want me to change and have me sent another version, or is it good to go in? -- Toon
Toon Claes <toon@iotcl.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Thanks for reviewing carefully. >> >> Both of two callers of unbundle_from_file() are used as the >> condition of an if() statement, so unbundle() that signals an error >> with -1 wouldn't be a problem, I would think. > > Hi Junio, > > I've noticed this patch wasn't picked up yet. Is there anything you want > me to change and have me sent another version, or is it good to go in? I am waiting for a reroll with an updated log message, i.e, what Patrick pointed out in his review. I only said "yeah, this looks safe", and never meant "it is so obvious there is no need for extra explanation in the log message". Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Both of two callers of unbundle_from_file() are used as the > condition of an if() statement, so unbundle() that signals an error > with -1 wouldn't be a problem, I would think. > It may not be a bad idea as a #leftoverbits item, after the dust > settles, to clean up the calling convention in this file (may not be > limited to the code path that reaches this function) to follow the > usual "signal success with 0, failures are signalled with a negative > value". Then we can just return the value we got from a failing > read_bundle_header(), just the same way we return the value we got > from a failing unbundle(). I just submitted v2 which preserves the original return values as before. I'll leave changing any return value to a #leftoverbits series to addresses them together. -- Toon
diff --git a/bundle-uri.c b/bundle-uri.c index 1e0ee156ba..eb49cba182 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -367,17 +367,21 @@ static int unbundle_from_file(struct repository *r, const char *file) struct strbuf bundle_ref = STRBUF_INIT; size_t bundle_prefix_len; - if ((bundle_fd = read_bundle_header(file, &header)) < 0) - return 1; + bundle_fd = read_bundle_header(file, &header); + if (bundle_fd < 0) { + result = 1; + goto cleanup; + } /* * Skip the reachability walk here, since we will be adding * a reachable ref pointing to the new tips, which will reach * the prerequisite commits. */ - if ((result = unbundle(r, &header, bundle_fd, NULL, - VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)))) - return 1; + result = unbundle(r, &header, bundle_fd, NULL, + VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)); + if (result) + goto cleanup; /* * Convert all refs/heads/ from the bundle into refs/bundles/ @@ -406,6 +410,8 @@ static int unbundle_from_file(struct repository *r, const char *file) 0, UPDATE_REFS_MSG_ON_ERR); } +cleanup: + strbuf_release(&bundle_ref); bundle_header_release(&header); return result; }
When the function returns early, the variable bundle_ref is not released through strbuf_release(). Fix this leak. And while at it, remove assignments in the conditions of the "if" statements as suggested in the CodingGuidelines. Signed-off-by: Toon Claes <toon@iotcl.com> --- bundle-uri.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) -- 2.46.0