From 34bd59992e9e11107d16837b671f867e2a5e77ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 2 May 2013 17:14:05 +0200 Subject: [PATCH 1/2] Revert "Protect sha1_entry_pos call with mutex" This reverts commit 8c535f3f6879c6796d8107d7eb80dd8b2105621b. --- src/pack.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/pack.c b/src/pack.c index 1ffad29ae..47534f195 100644 --- a/src/pack.c +++ b/src/pack.c @@ -1050,24 +1050,24 @@ static int pack_entry_find_offset( const git_oid *short_oid, size_t len) { - const uint32_t *level1_ofs; - const unsigned char *index; + const uint32_t *level1_ofs = p->index_map.data; + const unsigned char *index = p->index_map.data; unsigned hi, lo, stride; int pos, found = 0; const unsigned char *current = 0; *offset_out = 0; - if (!p->index_map.data && pack_index_open(p) < 0) - return git_odb__error_notfound("failed to open packfile", NULL); + if (index == NULL) { + int error; - if (git_mutex_lock(&p->lock) < 0) - return packfile_error("failed to get lock for finding entry offset"); + if ((error = pack_index_open(p)) < 0) + return error; + assert(p->index_map.data); - assert(p->index_map.data); - - index = p->index_map.data; - level1_ofs = p->index_map.data; + index = p->index_map.data; + level1_ofs = p->index_map.data; + } if (p->index_version > 1) { level1_ofs += 2; @@ -1093,8 +1093,6 @@ static int pack_entry_find_offset( /* Use git.git lookup code */ pos = sha1_entry_pos(index, stride, 0, lo, hi, p->num_objects, short_oid->id); - git_mutex_unlock(&p->lock); - if (pos >= 0) { /* An object matching exactly the oid was found */ found = 1; From 0ddfcb40d5ddf2e6d74f061efcccd944d18460cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 2 May 2013 18:06:14 +0200 Subject: [PATCH 2/2] Switch to index_version as "git_pack_file is ready" flag We use p->index_map.data to check whether the struct has been set up and all the information about the index is stored there. This variable gets set up halfway through the setup process, however, and a thread can come along and use fields that haven't been written to yet. Crucially, pack_entry_find_offset() needs to read the index version (which is written after index_map) to know the offset and stride length to pass to sha1_entry_pos(). If these values are wrong, assertions in it will fail, as it will be reading bogus data. Make index_version the last field to be written and switch from using p->index_map.data to p->index_version as "git_pack_file is ready" flag as we can use it to know if every field has been written. --- src/pack.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/pack.c b/src/pack.c index 47534f195..417d225f3 100644 --- a/src/pack.c +++ b/src/pack.c @@ -293,8 +293,8 @@ static int pack_index_check(const char *path, struct git_pack_file *p) } } - p->index_version = version; p->num_objects = nr; + p->index_version = version; return 0; } @@ -304,7 +304,7 @@ static int pack_index_open(struct git_pack_file *p) int error = 0; size_t name_len, base_len; - if (p->index_map.data) + if (p->index_version > -1) return 0; name_len = strlen(p->pack_name); @@ -320,7 +320,7 @@ static int pack_index_open(struct git_pack_file *p) if ((error = git_mutex_lock(&p->lock)) < 0) return error; - if (!p->index_map.data) + if (p->index_version == -1) error = pack_index_check(idx_name, p); git__free(idx_name); @@ -826,7 +826,7 @@ static int packfile_open(struct git_pack_file *p) git_oid sha1; unsigned char *idx_sha1; - if (!p->index_map.data && pack_index_open(p) < 0) + if (p->index_version == -1 && pack_index_open(p) < 0) return git_odb__error_notfound("failed to open packfile", NULL); /* if mwf opened by another thread, return now */ @@ -942,6 +942,7 @@ int git_packfile_alloc(struct git_pack_file **pack_out, const char *path) p->mwf.size = st.st_size; p->pack_local = 1; p->mtime = (git_time_t)st.st_mtime; + p->index_version = -1; git_mutex_init(&p->lock); @@ -1058,7 +1059,7 @@ static int pack_entry_find_offset( *offset_out = 0; - if (index == NULL) { + if (p->index_version == -1) { int error; if ((error = pack_index_open(p)) < 0)