Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Dec 17, 2013, 13:57 UTC
- Message-ID
- <CACsJy8DVc4soBrS1RbTLv93b5rere3htyL1DjRw=UcT4zVN9FA@mail.gmail.com>
- In-Reply-To
- <1387287838-25779-2-git-send-email-mhagger@alum.mit.edu>
On Tue, Dec 17, 2013 at 8:43 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 22 quoted lines
> 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.
Show 29 quoted lines
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> 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