Re: [PATCH] win32: ensure len does not cause any overreads

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Dec 19, 2022, 20:37 UTC
Message-ID: <dd47cbd0-35a6-1009-26e8-6a281224436d@dunelm.org.uk>
In-Reply-To: <pull.1404.git.git.1671470222521.gitgitgadget@gmail.com>

On 19/12/2022 17:17, Rose via GitGitGadget wrote:

Show 32 quoted lines

> From: Seija Kijin <doremylover123@gmail.com>
> 
> Check to make sure len is always less than MAX_PATH,
> otherwise an overread will occur, which is
> undefined behavior.
> 
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
>      win32: ensure len does not cause any overreads
>      
>      Check to make sure len is always less than MAX_PATH, otherwise an
>      overread will occur, which is undefined behavior.
>      
>      Signed-off-by: Seija Kijin doremylover123@gmail.com
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1404%2FAtariDreams%2Foverread-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AtariDreams/overread-v1
> Pull-Request: https://github.com/git/git/pull/1404
> 
>   compat/win32/dirent.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c
> index 52420ec7d4d..0c1bdccdd58 100644
> --- a/compat/win32/dirent.c
> +++ b/compat/win32/dirent.c
> @@ -27,7 +27,7 @@ DIR *opendir(const char *name)
>   	DIR *dir;
>   
>   	/* convert name to UTF-16 and check length < MAX_PATH */
> -	if ((len = xutftowcs_path(pattern, name)) < 0)
> +	if ((len = xutftowcs_path(pattern, name)) < 0 || len > MAX_PATH)

The documentation for xutftowcs_path() says

/**
  * Simplified file system specific variant of xutftowcsn, assumes output
  * buffer size is MAX_PATH wide chars and input string is \0-terminated,
  * fails with ENAMETOOLONG if input string is too long.
  */

Looking at the implementation it seems it does check the length so I don't think we need this change. I haven't looked into why 0217569bb2d (Win32: Unicode file name support (dirent), 2012-01-14) changed the length check from "len + 2 >= MAX_PATH" though.

Best Wishes

Phillip

Show 5 quoted lines

>   		return NULL;
>   
>   	/* append optional '/' and wildcard '*' */
> 
> base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7