blame: increment reference count for origin's commit#3842
Conversation
|
Ok, I revert my previous comment about no additional memory leaks. Seems like Valgrind just wasn't able to catch them. More fiddeling with refcounts then :/ |
When we create a blame origin, we try to look up the blob that is to be blamed at a certain revision. When this lookup fails, e.g. because the file did not exist at that certain revision, we fail to create the blame origin and return `NULL`. The blame origin that we have just allocated is thereby free'd with `origin_decref`. The `origin_decref` function does not only decrement reference counts for the blame origin, though, but also for its commit and blob. When this is done in the error case, we will cause an uneven reference count for these objects. This may result in hard-to-debug failures at seemingly unrelated code paths, where we try to access these objects when they in fact have already been free'd. Fix the issue by refactoring `make_origin` such that we only allocate the object after the only function that may fail so that we do not have to call `origin_decref` at all. Also fix the `pass_blame` function, which indirectly calls `make_origin`, to free the commit when `make_origin` failed.
|
Phew. I've finally been able to get to a version that has no memory leaks in our test suite while seemingly fixing the issues encountered in #3793. Running the modified example of @ameily does not cause a segfault after a few seconds anymore. I've been running the test on the libgit2 repo now for 20 minutes and until now everything looks good. These small issues are always a tad depressing when one looks at how long it took to fix related to the actual outcome, which most often looks rather trivial :/ But meh, whatever. |
Yeah, and blame is a special kind of terrible. :( As always, thanks. |
2 participants
blame: do not decrement commit refcount in make_origin
When we create a blame origin, we try to look up the blob that is
to be blamed at a certain revision. When this lookup fails, e.g.
because the file did not exist at that certain revision, we fail
to create the blame origin and return
NULL. The blame originthat we have just allocated is thereby free'd with
origin_decref.The
origin_decreffunction does not only decrement referencecounts for the blame origin, though, but also for its commit and
blob. When this is done in the error case, we will cause an
uneven reference count for these objects. This may result in
hard-to-debug failures at seemingly unrelated code paths, where
we try to access these objects when they in fact have already
been free'd.
Fix the issue by refactoring
make_originsuch that we onlyallocate the object after the only function that may fail so that
we do not have to call
origin_decrefat all. Also fix thepass_blamefunction, which indirectly callsmake_origin, tofree the commit when
make_originfailed.