Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
From: Duy Nguyen <hidden>
Date: 2016-06-15 22:59:28
On Tue, Dec 17, 2013 at 8:43 PM, Michael Haggerty [off-list ref] wrote:
The pathname character array might hold:
strlen(pathname) -- the pathname argument
'/' -- a slash, if not already present in pathname
%02x/ -- the first two characters of the SHA-1 plus slash
38 characters -- the last 38 characters of the SHA-1
NUL -- terminator
---------------------
strlen(pathname) + 43
(Actually, the NUL character is not written explicitly to pathname;
rather, the code relies on pathname being initialized to zeros and the
zero following the pathname still being there after the other
characters are written to the array.)
But the old pathname variable was PATH_MAX characters long, whereas
the check was (len > PATH_MAX - 42). So there might have been one
byte too many stored in pathname. This would have resulted in it's
not being NUL-terminated.
So, increase the size of the pathname variable by one byte to avoid
this possibility.Why don't we take this opportunity to replace that array with a strbuf? The conversion looks simple with this function.
quoted hunk ↗ jump to hunk
Signed-off-by: Michael Haggerty <redacted> --- builtin/prune-packed.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c index fa6ce42..81bc786 100644 --- a/builtin/prune-packed.c +++ b/builtin/prune-packed.c@@ -37,7 +37,7 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts) void prune_packed_objects(int opts) { int i; - static char pathname[PATH_MAX]; + static char pathname[PATH_MAX + 1]; const char *dir = get_object_directory(); int len = strlen(dir);@@ -45,7 +45,7 @@ void prune_packed_objects(int opts) progress = start_progress_delay("Removing duplicate objects", 256, 95, 2); - if (len > PATH_MAX - 42) + if (len + 42 > PATH_MAX) die("impossible object directory"); memcpy(pathname, dir, len); if (len && pathname[len-1] != '/') --
-- Duy