config_file: fix race when creating an iterator#5282
Merged
pks-t merged 5 commits intolibgit2:masterfrom Nov 5, 2019
Merged
Conversation
|
Does clar provide a facility to test async behavior? |
Member
Author
|
On Sat, Oct 26, 2019 at 01:04:24AM -0700, Erik Aigner wrote:
Does clar provide a facility to test async behavior?
Well, we do have some tests in "tests/threads", but that's about
it. Things is that "docs/threading.md" explicitly says:
Unless otherwise specified, libgit2 objects cannot be safely
accessed by multiple threads simultaneously.
Thus, things cannot be used from multiple threads at the same
time, at least officially. The truth is probably somewhat mixed,
as we tried to get some subsystems to behave just fine with
locking and refcounting. I'm always happy to fix any race
conditions as well, but one should always keep in mind our
official stance.
|
|
Yes, I know. Still, its hard to avoid race conditions if you are not using libgit in a commandline fashion, at which point one might just as well parse git output ;) |
eaigner
approved these changes
Oct 29, 2019
The configuration snapshot backend has been extracted from the old files backend back in 2bff84b (config_file: separate out read-only backend, 2019-07-26). To keep code churn manageable, the local functions weren't renamed yet and thus still have references to the old diskfile backend. Rename them accordingly to make them easier to understand.
As with the predecessing commit, this commit renames backend functions of the configuration file backend. This helps to clearly separate functionality and also to be able to see from backtraces which backend is currently in use.
The function to take a reference to the config file's config entries currently returns the reference via return value. Due to this, it's harder than necessary to integrate into our typical coding style, as one needs to make sure that a proper error code is set before erroring out from the caller. This bites us in `config_file_delete`, where we call `goto out` directly when `config_file_entries_take` returns `NULL`, but we actually forget to set up the error code and thus return success. Fix the issue by refactoring the function to return an error code and pass the reference via an out-pointer.
When creating a configuration file iterator, then we first refresh the backend and then afterwards duplicate all refreshed configuration entries into the iterator in order to avoid seeing any concurrent modifications of the entries while iterating. The duplication of entries is not guarded, though, as we do not increase the refcount of the entries that we duplicate right now. This opens us up for a race, as another thread may concurrently refresh the repository configuration and thus swap out the current set of entries. As we didn't increase the refcount, this may lead to the entries being free'd while we iterate over them in the first thread. Fix the issue by properly handling the lifecycle of the backend's entries via `config_file_entries_take` and `git_config_entries_free`, respectively.
63bc45e to
56b203a
Compare
Member
Author
|
Rebased to fix conflicts with #5293 |
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When creating a configuration file iterator, then we first refresh the
backend and then afterwards duplicate all refreshed configuration
entries into the iterator in order to avoid seeing any concurrent
modifications of the entries while iterating. The duplication of entries
is not guarded, though, as we do not increase the refcount of the
entries that we duplicate right now. This opens us up for a race, as
another thread may concurrently refresh the repository configuration and
thus swap out the current set of entries. As we didn't increase the
refcount, this may lead to the entries being free'd while we iterate
over them in the first thread.
Fix the issue by properly handling the lifecycle of the backend's
entries via
config_file_entries_takeandgit_config_entries_free,respectively.
I think this may be the likely cause of #5281, as it fixes a severe race condition in exactly the code path that #5281 shows.