libgit2/libgit2 — pull request #1720 by ethomson (2013-07-11 19:34:10+00:00)
Title: Thread safety in config file location cache

The config file location cache is not thread safe - populating the file locations is done on-demand, so the first time a repository tries to load its configs, these locations are populated. However using a static `git_buf` means that two threads entering these methods at the same time will do crazy things like `realloc` the buffers at the same time. This is quite easy to demonstrate with a program that simply spins up a number of repositories at once on different threads:

ntdll.dll!_RtlReportCriticalFailure@8() + 0x70 bytes
ntdll.dll!_RtlpReportHeapFailure@4() + 0x21 bytes
ntdll.dll!_RtlpLogHeapFailure@24() + 0xa2 bytes
ntdll.dll!_RtlReAllocateHeap@16() + 0x22170 bytes
git2.dll!realloc(void * pBlock, unsigned int newsize) Line 85 + 0x12 bytes C
git2.dll!git_buf_try_grow(git_buf * buf, unsigned int target_size, unsigned char mark_oom) Line 61 + 0x7 bytes C
git2.dll!git_buf_join(git_buf * buf, char separator, const char * str_a, const char * str_b) Line 412 + 0x17 bytes C
git2.dll!win32_find_existing_dirs(git_buf * out, const wchar_t * * tmpl, char * * temp) Line 174 + 0x14 bytes C
git2.dll!git_futils_find_global_file(git_buf * path, const char * filename) Line 734 + 0xd bytes C
git2.dll!git_repository_config__weakptr(git_config * * out, git_repository * repo) Line 524 + 0x5 bytes C
git2.dll!load_config_data(git_repository * repo) Line 137 + 0xa bytes C
git2.dll!git_repository_open_ext(git_repository * * repo_ptr, const char * start_path, unsigned int flags, const char * ceiling_dirs) Line 395 + 0x7 bytes C
git2.dll!git_repository_open(git_repository * * repo_out, const char * path) Line 410 C

So... this uses the same refcounting pattern that (say) `git_repository_odb` uses, but I feel like there's still a race condition here. Namely:

*out = foo;
GIT_REFCOUNT_INC(*out);

gives a time for another thread to free `*out`. Is the behavior here that we require users to set up their repositories on a single thread before using them? If so, I assume that we can extend this requirement to this config path configuration? Also if so, is this documented and I just didn't read it or should we make this more obvious?