I think this is probably an issue with unclear documentation, but since it's a crash, it seemed best to report it and get more knowledgeable eyes on it.
tl;dr
The "tl;dr" is that I think one of these three things is true, but I don't know which one, so I don't know how to approach a pull request for this:
- When using
git_config_iterator_new or git_config_multivar_iterator_new with git_config_next, you should never use git_config_entry_free, and the documentation should say this.
- When using
git_config_iterator_new or git_config_multivar_iterator_new with git_config_next, you should use git_config_entry_free only once, after the iterator loop, and the documentation should say this.
- When using
git_config_iterator_new or git_config_multivar_iterator_new with git_config_next, you should use git_config_entry_free on each iteration of the iterator loop, but there's a bug in git_config_entry_free that causes a crash, and that bug needs fixing.
Here are the details:
If you invoke git_config_next in a loop and invoke git_config_entry_free within that loop, the first loop iteration succeeds without issue, but on the second loop iteration, git_config_next appears to succeed (returns 0) and the program crashes when you try to use the entry.
Here's the code I'm talking about, which simply prints out everything from the global (user) config file:
#include <iostream>
#include <git2.h>
void
print_git_error(int error, char const * what)
{
git_error const * e = git_error_last();
std::cerr << "Falied to " << what << " due to git error " << error << ": "
<< (e ? e->message : "unknown") << std::endl;
git_libgit2_shutdown();
}
int
main()
{
git_libgit2_init();
git_config * cfg = nullptr;
int error = git_config_open_default(&cfg);
if (error < 0)
{
print_git_error(error, "open config");
return 1;
}
git_config_iterator * iter = nullptr;
error = git_config_iterator_new(&iter, cfg);
if (error < 0)
{
print_git_error(error, "create iterator");
git_config_free(cfg);
return 1;
}
while (true)
{
git_config_entry * entry = nullptr;
error = git_config_next(&entry, iter);
if (error < 0)
{
if (error != GIT_ITEROVER)
{
print_git_error(error, "call next on iterator");
git_config_entry_free(entry);
git_config_iterator_free(iter);
git_config_free(cfg);
return 1;
}
break;
}
std::cout << entry->name << " = " << (entry->value ? entry->value : "(null)") << std::endl;
git_config_entry_free(entry);
}
git_config_iterator_free(iter);
git_config_free(cfg);
git_libgit2_shutdown();
return 0;
}I compiled with this, with no errors or warnings:
$ g++ -std=c++20 -g -O0 -Wall -Wextra -Werror -fsanitize=address,undefined,leak -lgit2 -o git_config_entry_free_crash git_config_entry_free_crash.cpp
Here's the crash report, which I got from ASAN to have more information, but it still crashes without ASAN (just with less helpful information):
$ ASAN_OPTIONS=detect_stack_use_after_return=1:detect_leaks=1 ./git_config_entry_free_crash
user.name = Nick Williams
=================================================================
==25801==ERROR: AddressSanitizer: heap-use-after-free on address 0x7214225fa900 at pc 0x57e4f5554ad8 bp 0x7ffec58a0ab0 sp 0x7ffec58a0aa0
READ of size 8 at 0x7214225fa900 thread T0
#0 0x57e4f5554ad7 in main /home/williamsn/git_config_entry_free_crash.cpp:52
#1 0x75b423a2a600 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:59
#2 0x75b423a2a717 in __libc_start_main_impl ../csu/libc-start.c:360
#3 0x57e4f5554344 in _start (/home/williamsn/git_config_entry_free_crash+0x2344) (BuildId: 988ba5ece23bb80427cd53da49c89ac75a0a3cee)
0x7214225fa900 is located 0 bytes inside of 56-byte region [0x7214225fa900,0x7214225fa938)
freed by thread T0 here:
#0 0x75b424b2a3ff in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:51
#1 0x75b424906d3a (/usr/lib/x86_64-linux-gnu/libgit2.so.1.9+0x52d3a) (BuildId: 295ee292239dedf72a5937e9b603538eeb3817fe)
#2 0x57e4f5554d59 in main /home/williamsn/git_config_entry_free_crash.cpp:53
#3 0x75b423a2a600 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:59
#4 0x75b423a2a717 in __libc_start_main_impl ../csu/libc-start.c:360
#5 0x57e4f5554344 in _start (/home/williamsn/git_config_entry_free_crash+0x2344) (BuildId: 988ba5ece23bb80427cd53da49c89ac75a0a3cee)
previously allocated by thread T0 here:
#0 0x75b424b2b60f in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:67
#1 0x75b4248cd20c (/usr/lib/x86_64-linux-gnu/libgit2.so.1.9+0x1920c) (BuildId: 295ee292239dedf72a5937e9b603538eeb3817fe)
#2 0x75b424909101 (/usr/lib/x86_64-linux-gnu/libgit2.so.1.9+0x55101) (BuildId: 295ee292239dedf72a5937e9b603538eeb3817fe)
#3 0x75b424909223 (/usr/lib/x86_64-linux-gnu/libgit2.so.1.9+0x55223) (BuildId: 295ee292239dedf72a5937e9b603538eeb3817fe)
#4 0x75b4248fe487 (/usr/lib/x86_64-linux-gnu/libgit2.so.1.9+0x4a487) (BuildId: 295ee292239dedf72a5937e9b603538eeb3817fe)
#5 0x75b4248fc749 (/usr/lib/x86_64-linux-gnu/libgit2.so.1.9+0x48749) (BuildId: 295ee292239dedf72a5937e9b603538eeb3817fe)
#6 0x75b4248faf63 in git_config_next (/usr/lib/x86_64-linux-gnu/libgit2.so.1.9+0x46f63) (BuildId: 295ee292239dedf72a5937e9b603538eeb3817fe)
#7 0x57e4f5554971 in main /home/williamsn/git_config_entry_free_crash.cpp:39
#8 0x75b423a2a600 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:59
#9 0x75b423a2a717 in __libc_start_main_impl ../csu/libc-start.c:360
#10 0x57e4f5554344 in _start (/home/williamsn/git_config_entry_free_crash+0x2344) (BuildId: 988ba5ece23bb80427cd53da49c89ac75a0a3cee)
SUMMARY: AddressSanitizer: heap-use-after-free /home/williamsn/git_config_entry_free_crash.cpp:52 in main
Shadow bytes around the buggy address:
0x7214225fa680: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 00
0x7214225fa700: fa fa fa fa 00 00 00 00 00 00 00 fa fa fa fa fa
0x7214225fa780: 00 00 00 00 00 00 00 fa fa fa fa fa fd fd fd fd
0x7214225fa800: fd fd fd fd fa fa fa fa 00 00 00 00 00 00 00 00
0x7214225fa880: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
=>0x7214225fa900:[fd]fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa
0x7214225fa980: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7214225faa00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7214225faa80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7214225fab00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7214225fab80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==25801==ABORTING
Note that the location of the crash, git_config_entry_free_crash.cpp:52, is on the printing of the entry name and value. But it does print the first entry, and the ASAN report says that the memory was freed at git_config_entry_free_crash.cpp:53 (where git_config_entry_free is used), so the first loop iteration does complete successfully, and it is on the second loop iteration that it crashes.
Now, if I merely remove the git_config_entry_free on line 53 completely, it no longer crashes, prints all of my config vars, and exits with 0:
$ ASAN_OPTIONS=detect_stack_use_after_return=1:detect_leaks=1 ./git_config_entry_free_crash
user.name = Nick Williams
user.email = github@nicholaswilliams.net
Based on my theoretical explanation below, I actually expected this to leak one entry, but it did not leak at all. If I move the git_config_entry declaration to before the loop and the git_config_entry_free to after the loop, it also works without crashing or leaking, prints all of my config vars, and exits with 0. So I'm not sure which approach (if either) is "correct."
I think this is at least partially explained by this line from the documentation for git_config_next:
The pointers returned by this function are valid until the next call to git_config_next or until the iterator is freed.
I believe this means that the underlying backend iterator is expected to use the same git_config_entry buffer for each call to git_config_next. If this belief is correct, then by calling git_config_entry_free inside the loop, I have corrupted that buffer. But, in that case, the documentation is woefully inadequate, and should spell out that the entry should not be freed until after the loop. However, I may be wrong, and this may not be the intended behavior.
It's also unclear how this interacts with backends. The behavior I'm describing all involves the file backend, as the public git_config_next merely delegates to that, but other backends could be implemented (such as an in-memory backend at level GIT_CONFIG_LEVEL_APP), and backends should handle memory consistently with each other. Is it required behavior that backends re-use a single git_config_entry buffer and take care of freeing that buffer so that the application doesn't have to? If so, that should be documented.
I think this is probably an issue with unclear documentation, but since it's a crash, it seemed best to report it and get more knowledgeable eyes on it.
tl;dr
The "tl;dr" is that I think one of these three things is true, but I don't know which one, so I don't know how to approach a pull request for this:
git_config_iterator_neworgit_config_multivar_iterator_newwithgit_config_next, you should never usegit_config_entry_free, and the documentation should say this.git_config_iterator_neworgit_config_multivar_iterator_newwithgit_config_next, you should usegit_config_entry_freeonly once, after the iterator loop, and the documentation should say this.git_config_iterator_neworgit_config_multivar_iterator_newwithgit_config_next, you should usegit_config_entry_freeon each iteration of the iterator loop, but there's a bug ingit_config_entry_freethat causes a crash, and that bug needs fixing.Here are the details:
If you invoke
git_config_nextin a loop and invokegit_config_entry_freewithin that loop, the first loop iteration succeeds without issue, but on the second loop iteration,git_config_nextappears to succeed (returns 0) and the program crashes when you try to use the entry.Here's the code I'm talking about, which simply prints out everything from the global (user) config file:
I compiled with this, with no errors or warnings:
Here's the crash report, which I got from ASAN to have more information, but it still crashes without ASAN (just with less helpful information):
Note that the location of the crash,
git_config_entry_free_crash.cpp:52, is on the printing of the entry name and value. But it does print the first entry, and the ASAN report says that the memory was freed atgit_config_entry_free_crash.cpp:53(wheregit_config_entry_freeis used), so the first loop iteration does complete successfully, and it is on the second loop iteration that it crashes.Now, if I merely remove the
git_config_entry_freeon line 53 completely, it no longer crashes, prints all of my config vars, and exits with 0:Based on my theoretical explanation below, I actually expected this to leak one entry, but it did not leak at all. If I move the
git_config_entrydeclaration to before the loop and thegit_config_entry_freeto after the loop, it also works without crashing or leaking, prints all of my config vars, and exits with 0. So I'm not sure which approach (if either) is "correct."I think this is at least partially explained by this line from the documentation for
git_config_next:I believe this means that the underlying backend iterator is expected to use the same
git_config_entrybuffer for each call togit_config_next. If this belief is correct, then by callinggit_config_entry_freeinside the loop, I have corrupted that buffer. But, in that case, the documentation is woefully inadequate, and should spell out that the entry should not be freed until after the loop. However, I may be wrong, and this may not be the intended behavior.It's also unclear how this interacts with backends. The behavior I'm describing all involves the file backend, as the public
git_config_nextmerely delegates to that, but other backends could be implemented (such as an in-memory backend at levelGIT_CONFIG_LEVEL_APP), and backends should handle memory consistently with each other. Is it required behavior that backends re-use a singlegit_config_entrybuffer and take care of freeing that buffer so that the application doesn't have to? If so, that should be documented.