patchwin32: ensure len does not cause any overreads

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(-)

On Mon, Dec 19 2022, Rose via GitGitGadget wrote:

We tend to avoid assignments in "if", I think before this change it
could have passed, but now that we have a more complex expression it's
worth splitting it out. So, we can just move it up to when "int" is declared:
	
	diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c
	index 52420ec7d4d..bf371cc9714 100644
	--- a/compat/win32/dirent.c
	+++ b/compat/win32/dirent.c
	@@ -23,11 +23,11 @@ DIR *opendir(const char *name)
	 	wchar_t pattern[MAX_PATH + 2]; /* + 2 for '/' '*' */
	 	WIN32_FIND_DATAW fdata;
	 	HANDLE h;
	-	int len;
	+	int len = xutftowcs_path(pattern, name);
	 	DIR *dir;
	 
	 	/* convert name to UTF-16 and check length < MAX_PATH */
	-	if ((len = xutftowcs_path(pattern, name)) < 0)
	+	if (len < 0 || len > MAX_PATH)
	 		return NULL;
	 
	 	/* append optional '/' and wildcard '*' */

But that leaves the question of whether this was just omitted from 0217569bb2d (Win32: Unicode file name support (dirent), 2012-01-14) by mistake?

The comment above the code you're tweaking says we're checking that "length < MAX_PATH", but as we can see 0217569bb2d dropped that condition.

So, was that a bug? And if so why is your check for MAX_PATH different than its check?

Shouldn't yours be (as it did):

	if (len + 2 >= MAX_PATH) {
		errno = ENAMETOOLONG;
		return NULL;
	}

?

Perhaps not, but the commit message should discuss it, i.e. why is the MAX_PATH check now subtly different than the pre-0217569bb2d one.q

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

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

From: Seija Kijin <doremylover123@gmail.com>

Check to make sure len is always two 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%2FAreaZR%2Foverread-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AreaZR/overread-v2
Pull-Request: https://github.com/git/git/pull/1404

Range-diff vs v1:

 1:  f9ec5429d01 ! 1:  dfc34fb4c1a win32: ensure len does not cause any overreads
     @@ Metadata
       ## Commit message ##
          win32: ensure len does not cause any overreads
      
     -    Check to make sure len is always less than MAX_PATH,
     +    Check to make sure len is always two less than MAX_PATH,
          otherwise an overread will occur, which is
          undefined behavior.
      
     @@ Commit message
      
       ## compat/win32/dirent.c ##
      @@ compat/win32/dirent.c: 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)
     + 	if ((len = xutftowcs_path(pattern, name)) < 0)
       		return NULL;
       
     ++	if (len + 2 >= MAX_PATH)
     ++		return NULL;
     ++
       	/* append optional '/' and wildcard '*' */
     + 	if (len && !is_dir_sep(pattern[len - 1]))
     + 		pattern[len++] = '/';

 compat/win32/dirent.c | 3 +++
 1 file changed, 3 insertions(+)