Skip to content

Commit

Permalink
index: convert read_entry to return entry size via an out-param
Browse files Browse the repository at this point in the history
The function `read_entry` does not conform to our usual coding style of
returning stuff via the out parameter and to use the return value for
reporting errors. Due to most of our code conforming to that pattern, it
has become quite natural for us to actually return `-1` in case there is
any error, which has also slipped in with commit 5625d86 (index:
support index v4, 2016-05-17). As the function returns an `size_t` only,
though, the return value is wrapped around, causing the caller of
`read_tree` to continue with an invalid index entry. Ultimately, this
can lead to a double-free.

Improve code and fix the bug by converting the function to return the
index entry size via an out parameter and only using the return value to
indicate errors.

Reported-by: Krishna Ram Prakash R <krp@gtux.in>
Reported-by: Vivek Parikh <viv0411.parikh@gmail.com>
  • Loading branch information
pks-t committed Mar 10, 2018
1 parent d11c4a1 commit 58a6fe9
Showing 1 changed file with 13 additions and 9 deletions.
22 changes: 13 additions & 9 deletions src/index.c
Original file line number Diff line number Diff line change
Expand Up @@ -2299,8 +2299,9 @@ static size_t index_entry_size(size_t path_len, size_t varint_len, uint32_t flag
}
}

static size_t read_entry(
static int read_entry(
git_index_entry **out,
size_t *out_size,
git_index *index,
const void *buffer,
size_t buffer_size,
Expand All @@ -2314,7 +2315,7 @@ static size_t read_entry(
char *tmp_path = NULL;

if (INDEX_FOOTER_SIZE + minimal_entry_size > buffer_size)
return 0;
return -1;

/* buffer is not guaranteed to be aligned */
memcpy(&source, buffer, sizeof(struct entry_short));
Expand Down Expand Up @@ -2356,7 +2357,7 @@ static size_t read_entry(

path_end = memchr(path_ptr, '\0', buffer_size);
if (path_end == NULL)
return 0;
return -1;

path_length = path_end - path_ptr;
}
Expand Down Expand Up @@ -2386,16 +2387,20 @@ static size_t read_entry(
entry.path = tmp_path;
}

if (entry_size == 0)
return -1;

if (INDEX_FOOTER_SIZE + entry_size > buffer_size)
return 0;
return -1;

if (index_entry_dup(out, index, &entry) < 0) {
git__free(tmp_path);
return 0;
return -1;
}

git__free(tmp_path);
return entry_size;
*out_size = entry_size;
return 0;
}

static int read_header(struct index_header *dest, const void *buffer)
Expand Down Expand Up @@ -2499,10 +2504,9 @@ static int parse_index(git_index *index, const char *buffer, size_t buffer_size)
/* Parse all the entries */
for (i = 0; i < header.entry_count && buffer_size > INDEX_FOOTER_SIZE; ++i) {
git_index_entry *entry = NULL;
size_t entry_size = read_entry(&entry, index, buffer, buffer_size, last);
size_t entry_size;

/* 0 bytes read means an object corruption */
if (entry_size == 0) {
if ((error = read_entry(&entry, &entry_size, index, buffer, buffer_size, last)) < 0) {
error = index_error_invalid("invalid entry");
goto done;
}
Expand Down

0 comments on commit 58a6fe9

Please sign in to comment.