New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix use-after-free when patch outlives diff #461
Conversation
I think we should go all-or-nothing on copying the data or relying on the libgit2 object to hold it. The intention of the rest of the code is clearly to copy the data, so we should do the same for the paths, be it via |
|
blame is very expensive regardless. Two If you're building blame in python instead of using the libgit2 blame, you're already giving up a lot more performance/resources than these dups are going to cost. If you're worried about triggering a syscall (since you don't seem to have an issue with the (at the very least) two extra allocations just a few lines above), you can use the python allocator, which is likely to have a bunch of memory laying around (but then again, so is the libc allocator). |
Fair point. I'm not implementing |
Any other concerns? |
@@ -129,7 +129,6 @@ wrap_patch(git_patch *patch) | |||
} | |||
} | |||
} | |||
git_patch_free(patch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we leaking the patch now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed.
I do wonder how much sense adding this test makes. It's testing these two fields, which we now know won't cause an error, but it's not checking anything else, and we can't tell from the test itself whether something has gone wrong, but we need an external tool. |
I agree, the test is a bit awkward - advice here would be appreciated. I don't see a precedent for adding C tests in this repo. If so, I could add a test that overrides |
So what do you want to see here? Boot the test? Squash the commits to just one that adds |
I'd remove it. Any memory issues need to be looked at with a custom version of python to boot, so I don't see that this running in the normal case brings much. |
Test removed, commits squashed, and rebased on master. |
Thanks! |
Hi @garious Just noticed that this PR has introduced compilation warnings, could you fix them?
|
Fixed: #474 |
And added test that would trigger the bug in Valgrind.