| Message ID | pull.1404.git.git.1671470222521.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | win32: ensure len does not cause any overreads | expand |
On Mon, Dec 19 2022, Rose via GitGitGadget wrote: > 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) 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: > 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 > return NULL; > > /* append optional '/' and wildcard '*' */ > > base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
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) return NULL; /* append optional '/' and wildcard '*' */