| Message ID | 31e38ba4e150c9bc9e3aa1073869881ccba9035e.1723540931.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.4) | expand |
Patrick Steinhardt <ps@pks.im> writes: > We populate a `struct symdiff` in case the user has requested a > symmetric diff. Part of this is to populate a `skip` bitmap that > indicates which commits shall be ignored in the diff. But while this > bitmap is dynamically allocated, we never free it. > > Fix this by introducing and calling a new `symdiff_release()` function > that does this for us. OK. > +static void symdiff_release(struct symdiff *sdiff) > +{ > + if (!sdiff) > + return; > + bitmap_free(sdiff->skip); > +} Hmph, wouldn't it be a BUG if any caller feeds a NULL pointer to it, though? When symdiff was prepared but not used, sdiff->skip will be NULL but sdiff is never NULL even in such a case. > @@ -398,7 +405,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) > struct object_array_entry *blob[2]; > int nongit = 0, no_index = 0; > int result; > - struct symdiff sdiff; > + struct symdiff sdiff = {0}; And symdiff_prepare() at least clears its .skip member to NULL, so this pre-initialization is probably not needed. If we are preparing ourselves for future changes of the flow in this function (e.g. goto's that jump to the clean-up label from which symdiff_release() is always called, even when we did not call symdiff_prepare() on this thing), this is probably not sufficient to convey that intention (instead I'd use an explicit ".skip = NULL" to say "we might not even call _prepare() but this one is prepared to be passed to _release() even in such a case"). Given that there is no such goto exists, and that _prepare() always sets up the .skip member appropriately, I wonder if we are much better off leaving sdiff uninitialized at the declaration site here. If we add such a goto that bypasses _prepare() in the future, the compiler will notice that we are passing an uninitialized sdiff to _release(), no? > @@ -619,6 +626,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) > refresh_index_quietly(); > release_revisions(&rev); > object_array_clear(&ent); > + symdiff_release(&sdiff); > UNLEAK(blob); > return result; > } Other than that, this looks cleanly done. Thanks.
On Tue, Aug 13, 2024 at 09:25:41AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > We populate a `struct symdiff` in case the user has requested a > > symmetric diff. Part of this is to populate a `skip` bitmap that > > indicates which commits shall be ignored in the diff. But while this > > bitmap is dynamically allocated, we never free it. > > > > Fix this by introducing and calling a new `symdiff_release()` function > > that does this for us. > > OK. > > > +static void symdiff_release(struct symdiff *sdiff) > > +{ > > + if (!sdiff) > > + return; > > + bitmap_free(sdiff->skip); > > +} > > Hmph, wouldn't it be a BUG if any caller feeds a NULL pointer to it, > though? When symdiff was prepared but not used, sdiff->skip will be > NULL but sdiff is never NULL even in such a case. Good point. It does make sense for `_free()` functions to handle NULL pointers, but doesn't quite for `_release()` ones. > > @@ -398,7 +405,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) > > struct object_array_entry *blob[2]; > > int nongit = 0, no_index = 0; > > int result; > > - struct symdiff sdiff; > > + struct symdiff sdiff = {0}; > > And symdiff_prepare() at least clears its .skip member to NULL, so > this pre-initialization is probably not needed. If we are preparing > ourselves for future changes of the flow in this function (e.g. > goto's that jump to the clean-up label from which symdiff_release() > is always called, even when we did not call symdiff_prepare() on > this thing), this is probably not sufficient to convey that > intention (instead I'd use an explicit ".skip = NULL" to say "we > might not even call _prepare() but this one is prepared to be passed > to _release() even in such a case"). > > Given that there is no such goto exists, and that _prepare() always > sets up the .skip member appropriately, I wonder if we are much > better off leaving sdiff uninitialized at the declaration site here. > If we add such a goto that bypasses _prepare() in the future, the > compiler will notice that we are passing an uninitialized sdiff to > _release(), no? You'd hope it does, but it certainly depends on your compiler flags. Various hardening flags for example implicitly initialize variables, and I have a feeling that this also causes them to not emit any warnings anymore. At least I only spot such warnings in CI. In any case, yes, we can drop the initialization here. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Good point. It does make sense for `_free()` functions to handle NULL > pointers, but doesn't quite for `_release()` ones. I agree that foo_free() should accept NULL and silently become a no-op. I do not care deeply whether foo_release() did the same, or not, as long as all *_release()s behave the same way. Maybe it is more convenient if they ignored NULL, as I have a hunch that feeding a NULL pointer to foo_release() is unlikely to be a bug. Since we documented our aspiration to use these (and foo_clear()) consistently, we may #leftoverbits want to also document the calling convention as well. >> And symdiff_prepare() at least clears its .skip member to NULL, so >> this pre-initialization is probably not needed. If we are preparing >> ourselves for future changes of the flow in this function (e.g. >> goto's that jump to the clean-up label from which symdiff_release() >> is always called, even when we did not call symdiff_prepare() on >> this thing), this is probably not sufficient to convey that >> intention (instead I'd use an explicit ".skip = NULL" to say "we >> might not even call _prepare() but this one is prepared to be passed >> to _release() even in such a case"). >> >> Given that there is no such goto exists, and that _prepare() always >> sets up the .skip member appropriately, I wonder if we are much >> better off leaving sdiff uninitialized at the declaration site here. >> If we add such a goto that bypasses _prepare() in the future, the >> compiler will notice that we are passing an uninitialized sdiff to >> _release(), no? > > You'd hope it does, but it certainly depends on your compiler flags. > Various hardening flags for example implicitly initialize variables, and > I have a feeling that this also causes them to not emit any warnings > anymore. At least I only spot such warnings in CI. Yeah, that is a sad fact in the real world X-<. To be defensive, I think an explicit "{ .skip = NULL }" or "{ 0 }" would not be too bad and may even serve as a good reminder for developers who may want to jump over the call to _prepare() in the future. The explicit ".skip = NULL" says "we know it is safe to call _release() with a struct that hasn't gone through _prepare(), as long as its .skip member is cleared", but the story "{ 0 }" tells us is not much more than "we clear just like everybody else", and that is why I suggested the former (iow, I know both mean the same thing to the C compiler---I just care more about what it tells the human readers). Thanks.
diff --git a/builtin/diff.c b/builtin/diff.c index 9b6cdabe15..f87f68a5bc 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -388,6 +388,13 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym) sym->skip = map; } +static void symdiff_release(struct symdiff *sdiff) +{ + if (!sdiff) + return; + bitmap_free(sdiff->skip); +} + int cmd_diff(int argc, const char **argv, const char *prefix) { int i; @@ -398,7 +405,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) struct object_array_entry *blob[2]; int nongit = 0, no_index = 0; int result; - struct symdiff sdiff; + struct symdiff sdiff = {0}; /* * We could get N tree-ish in the rev.pending_objects list. @@ -619,6 +626,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) refresh_index_quietly(); release_revisions(&rev); object_array_clear(&ent); + symdiff_release(&sdiff); UNLEAK(blob); return result; } diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh index eff63c16b0..4d6565e728 100755 --- a/t/t4068-diff-symmetric-merge-base.sh +++ b/t/t4068-diff-symmetric-merge-base.sh @@ -5,6 +5,7 @@ test_description='behavior of diff with symmetric-diff setups and --merge-base' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # build these situations: diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh index c558282bc0..3211e1e65f 100755 --- a/t/t4108-apply-threeway.sh +++ b/t/t4108-apply-threeway.sh @@ -5,6 +5,7 @@ test_description='git apply --3way' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh print_sanitized_conflicted_diff () {
We populate a `struct symdiff` in case the user has requested a symmetric diff. Part of this is to populate a `skip` bitmap that indicates which commits shall be ignored in the diff. But while this bitmap is dynamically allocated, we never free it. Fix this by introducing and calling a new `symdiff_release()` function that does this for us. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/diff.c | 10 +++++++++- t/t4068-diff-symmetric-merge-base.sh | 1 + t/t4108-apply-threeway.sh | 1 + 3 files changed, 11 insertions(+), 1 deletion(-)