Skip to content

pack: fix race in pack_entry_find_offset#3984

Merged
carlosmn merged 1 commit into
libgit2:masterfrom
pks-t:pks/pack-find-offset-race
Nov 2, 2016
Merged

pack: fix race in pack_entry_find_offset#3984
carlosmn merged 1 commit into
libgit2:masterfrom
pks-t:pks/pack-find-offset-race

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Nov 2, 2016

In pack_entry_find_offset, we try to find the offset of a
certain object in the pack file. To do so, we first assert if the
packfile has already been opened and open it if not. Opening the
packfile is guarded with a mutex, so concurrent access to this is
in fact safe.

What is not thread-safe though is our calculation of offsets
inside the packfile. Assume two threads calling
pack_entry_find_offset at the same time. We first calculate the
offset and index location and only then determine if the pack has
already been opened. If so, we re-calculate the offset and index
address.

Now the case for two threads: thread 1 first calculates the
addresses and is subsequently suspended. The second thread will
now call pack_index_open and initialize the pack file,
calculating its addresses correctly. When the first thread is
resumed now, he'll see that the pack file has already been
initialized and will happily proceed with the addresses it has
already calculated before the check. As the pack file was not
initialized before, these addresses are bogus.

Fix the issue by only calculating the addresses after having
checked if the pack file is open.

In `pack_entry_find_offset`, we try to find the offset of a
certain object in the pack file. To do so, we first assert if the
packfile has already been opened and open it if not. Opening the
packfile is guarded with a mutex, so concurrent access to this is
in fact safe.

What is not thread-safe though is our calculation of offsets
inside the packfile. Assume two threads calling
`pack_entry_find_offset` at the same time. We first calculate the
offset and index location and only then determine if the pack has
already been opened. If so, we re-calculate the offset and index
address.

Now the case for two threads: thread 1 first calculates the
addresses and is subsequently suspended. The second thread will
now call `pack_index_open` and initialize the pack file,
calculating its addresses correctly. When the first thread is
resumed now, he'll see that the pack file has already been
initialized and will happily proceed with the addresses it has
already calculated before the check. As the pack file was not
initialized before, these addresses are bogus.

Fix the issue by only calculating the addresses after having
checked if the pack file is open.
@carlosmn
Copy link
Copy Markdown
Member

carlosmn commented Nov 2, 2016

Thanks for finding this.

@carlosmn carlosmn merged commit d2451fe into libgit2:master Nov 2, 2016
@pks-t pks-t deleted the pks/pack-find-offset-race branch February 8, 2017 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants