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 strdup() or allocating a python string.

strdup is what I started with locally. I changed it to what's in this patch because using strdup means there's two extra mallocs per file per patch. If you're trying to implement something like git blame with PyGit2, using strdup could trigger a lot of extra mallocs.

blame is very expensive regardless. Two strdup calls here are not going to change the balance and by doing this, you're keeping more memory alive, which means any other malloc is going to be more expensive.

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 git blame, just trying to offer a concrete case that could be affected by this patch. Sounds to me like my original patch is a case of premature optimization. I've added a second patch that uses strdup.

Any other concerns?

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 free and sets the freed memory to some expected value. And it looks like the ability to intercept Python allocations wasn't added until Python 3.4. So the best I could think of was to demonstrate the code path where the use-after-free occurs. You won't see an error in Python unless the freed heap space is reused before it is accessed. I couldn't think of a way to reliably do that, so I figured the next best thing was to add a test that will trigger an error in Valgrind if my patch is not present. If you feel this test just adds confusion, I'd be happy to remove it.

So what do you want to see here? Boot the test? Squash the commits to just one that adds strdup and free? I'd love to get something merged sooner than later so that I can delete my local fork. What needs to happen to move forward?

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?

x86_64-pc-linux-gnu-gcc -pthread -fPIC -I/usr/local/include -I/usr/include/python2.7 -c src/diff.c -o build/temp.linux-x86_64-2.7/src/diff.o
src/diff.c: In function ‘Patch_dealloc’:
src/diff.c:156:5: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [enabled by default]
     free(self->old_file_path);
     ^
In file included from /usr/include/python2.7/Python.h:42:0,
                 from src/diff.c:29:
/usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type ‘const char *’
 extern void free (void *__ptr) __THROW;
             ^
src/diff.c:157:5: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [enabled by default]
     free(self->new_file_path);
     ^
In file included from /usr/include/python2.7/Python.h:42:0,
             from src/diff.c:29:
/usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type ‘const char *’
 extern void free (void *__ptr) __THROW;
             ^

Fixed: #474