{"thread":{"id":"58982","subject":"[PATCH] win32: ensure len does not cause any overreads","startedAt":"2022-12-19T17:17:16Z","lastAt":"2024-12-18T00:29:53Z","messageCount":4,"participants":["Rose via GitGitGadget","Ævar Arnfjörð Bjarmason","Phillip Wood","AreaZR via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469301","messageId":"pull.1404.git.git.1671470222521.gitgitgadget@gmail.com","threadId":"58982","inReplyTo":null,"subject":"[PATCH] win32: ensure len does not cause any overreads","fromName":"Rose via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-19T17:17:02Z","receivedAt":"2022-12-19T17:17:16Z","isPatch":true,"sender":{"key":"ckelsch@jgrcpa.com","avatar":null},"body":"From: Seija Kijin \n\nCheck to make sure len is always less than MAX_PATH,\notherwise an overread will occur, which is\nundefined behavior.\n\nSigned-off-by: Seija Kijin \n---\n win32: ensure len does not cause any overreads\n \n Check to make sure len is always less than MAX_PATH, otherwise an\n overread will occur, which is undefined behavior.\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1404%2FAtariDreams%2Foverread-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AtariDreams/overread-v1\nPull-Request: https://github.com/git/git/pull/1404\n\n compat/win32/dirent.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/win32/dirent.c b/compat/win32/dirent.c\nindex 52420ec7d4d..0c1bdccdd58 100644\n--- a/compat/win32/dirent.c\n+++ b/compat/win32/dirent.c\n@@ -27,7 +27,7 @@ DIR *opendir(const char *name)\n \tDIR *dir;\n \n \t/* convert name to UTF-16 and check length < MAX_PATH */\n-\tif ((len = xutftowcs_path(pattern, name)) < 0)\n+\tif ((len = xutftowcs_path(pattern, name)) < 0 || len > MAX_PATH)\n \t\treturn NULL;\n \n \t/* append optional '/' and wildcard '*' */\n\nbase-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7\n-- \ngitgitgadget\n"},{"id":"469304","messageId":"221219.86v8m7xncc.gmgdl@evledraar.gmail.com","threadId":"58982","inReplyTo":"pull.1404.git.git.1671470222521.gitgitgadget@gmail.com","subject":"Re: [PATCH] win32: ensure len does not cause any overreads","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T18:19:42Z","receivedAt":"2022-12-19T18:25:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 19 2022, Rose via GitGitGadget wrote:\n\n> From: Seija Kijin \n>\n> Check to make sure len is always less than MAX_PATH,\n> otherwise an overread will occur, which is\n> undefined behavior.\n>\n> Signed-off-by: Seija Kijin \n> ---\n> win32: ensure len does not cause any overreads\n> \n> Check to make sure len is always less than MAX_PATH, otherwise an\n> overread will occur, which is undefined behavior.\n> \n> Signed-off-by: Seija Kijin doremylover123@gmail.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1404%2FAtariDreams%2Foverread-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AtariDreams/overread-v1\n> Pull-Request: https://github.com/git/git/pull/1404\n>\n> compat/win32/dirent.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c\n> index 52420ec7d4d..0c1bdccdd58 100644\n> --- a/compat/win32/dirent.c\n> +++ b/compat/win32/dirent.c\n> @@ -27,7 +27,7 @@ DIR *opendir(const char *name)\n> \tDIR *dir;\n> \n> \t/* convert name to UTF-16 and check length < MAX_PATH */\n> -\tif ((len = xutftowcs_path(pattern, name)) < 0)\n> +\tif ((len = xutftowcs_path(pattern, name)) < 0 || len > MAX_PATH)\n\nWe tend to avoid assignments in \"if\", I think before this change it\ncould have passed, but now that we have a more complex expression it's\nworth splitting it out. So, we can just move it up to when \"int\" is declared:\n\t\n\tdiff --git a/compat/win32/dirent.c b/compat/win32/dirent.c\n\tindex 52420ec7d4d..bf371cc9714 100644\n\t--- a/compat/win32/dirent.c\n\t+++ b/compat/win32/dirent.c\n\t@@ -23,11 +23,11 @@ DIR *opendir(const char *name)\n\t \twchar_t pattern[MAX_PATH + 2]; /* + 2 for '/' '*' */\n\t \tWIN32_FIND_DATAW fdata;\n\t \tHANDLE h;\n\t-\tint len;\n\t+\tint len = xutftowcs_path(pattern, name);\n\t \tDIR *dir;\n\t \n\t \t/* convert name to UTF-16 and check length < MAX_PATH */\n\t-\tif ((len = xutftowcs_path(pattern, name)) < 0)\n\t+\tif (len < 0 || len > MAX_PATH)\n\t \t\treturn NULL;\n\t \n\t \t/* append optional '/' and wildcard '*' */\n\nBut that leaves the question of whether this was just omitted from\n0217569bb2d (Win32: Unicode file name support (dirent), 2012-01-14) by\nmistake?\n\nThe comment above the code you're tweaking says we're checking that\n\"length < MAX_PATH\", but as we can see 0217569bb2d dropped that\ncondition.\n\nSo, was that a bug? And if so why is your check for MAX_PATH different\nthan its check?\n\nShouldn't yours be (as it did):\n\n\tif (len + 2 >= MAX_PATH) {\n\t\terrno = ENAMETOOLONG;\n\t\treturn NULL;\n\t}\n\n?\n\nPerhaps not, but the commit message should discuss it, i.e. why is the\nMAX_PATH check now subtly different than the pre-0217569bb2d one.q\n"},{"id":"469316","messageId":"dd47cbd0-35a6-1009-26e8-6a281224436d@dunelm.org.uk","threadId":"58982","inReplyTo":"pull.1404.git.git.1671470222521.gitgitgadget@gmail.com","subject":"Re: [PATCH] win32: ensure len does not cause any overreads","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-19T20:37:07Z","receivedAt":"2022-12-19T20:37:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 19/12/2022 17:17, Rose via GitGitGadget wrote:\n> From: Seija Kijin \n> \n> Check to make sure len is always less than MAX_PATH,\n> otherwise an overread will occur, which is\n> undefined behavior.\n> \n> Signed-off-by: Seija Kijin \n> ---\n> win32: ensure len does not cause any overreads\n> \n> Check to make sure len is always less than MAX_PATH, otherwise an\n> overread will occur, which is undefined behavior.\n> \n> Signed-off-by: Seija Kijin doremylover123@gmail.com\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1404%2FAtariDreams%2Foverread-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AtariDreams/overread-v1\n> Pull-Request: https://github.com/git/git/pull/1404\n> \n> compat/win32/dirent.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c\n> index 52420ec7d4d..0c1bdccdd58 100644\n> --- a/compat/win32/dirent.c\n> +++ b/compat/win32/dirent.c\n> @@ -27,7 +27,7 @@ DIR *opendir(const char *name)\n> \tDIR *dir;\n> \n> \t/* convert name to UTF-16 and check length < MAX_PATH */\n> -\tif ((len = xutftowcs_path(pattern, name)) < 0)\n> +\tif ((len = xutftowcs_path(pattern, name)) < 0 || len > MAX_PATH)\n\nThe documentation for xutftowcs_path() says\n\n/**\n * Simplified file system specific variant of xutftowcsn, assumes output\n * buffer size is MAX_PATH wide chars and input string is \\0-terminated,\n * fails with ENAMETOOLONG if input string is too long.\n */\n\nLooking at the implementation it seems it does check the length so I \ndon't think we need this change. I haven't looked into why 0217569bb2d \n(Win32: Unicode file name support (dirent), 2012-01-14) changed the \nlength check from \"len + 2 >= MAX_PATH\" though.\n\nBest Wishes\n\nPhillip\n\n> \t\treturn NULL;\n> \n> \t/* append optional '/' and wildcard '*' */\n> \n> base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7\n"},{"id":"509248","messageId":"pull.1404.v2.git.git.1734481790015.gitgitgadget@gmail.com","threadId":"58982","inReplyTo":"pull.1404.git.git.1671470222521.gitgitgadget@gmail.com","subject":"[PATCH v2] win32: ensure len does not cause any overreads","fromName":"AreaZR via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-18T00:29:49Z","receivedAt":"2024-12-18T00:29:53Z","isPatch":true,"sender":{"key":"name:AreaZR","avatar":null},"body":"From: Seija Kijin \n\nCheck to make sure len is always two less than MAX_PATH,\notherwise an overread will occur, which is\nundefined behavior.\n\nSigned-off-by: Seija Kijin \n---\n win32: ensure len does not cause any overreads\n \n Check to make sure len is always less than MAX_PATH, otherwise an\n overread will occur, which is undefined behavior.\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1404%2FAreaZR%2Foverread-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1404/AreaZR/overread-v2\nPull-Request: https://github.com/git/git/pull/1404\n\nRange-diff vs v1:\n\n 1: f9ec5429d01 ! 1: dfc34fb4c1a win32: ensure len does not cause any overreads\n @@ Metadata\n ## Commit message ##\n win32: ensure len does not cause any overreads\n \n - Check to make sure len is always less than MAX_PATH,\n + Check to make sure len is always two less than MAX_PATH,\n otherwise an overread will occur, which is\n undefined behavior.\n \n @@ Commit message\n \n ## compat/win32/dirent.c ##\n @@ compat/win32/dirent.c: DIR *opendir(const char *name)\n - \tDIR *dir;\n - \n - \t/* convert name to UTF-16 and check length < MAX_PATH */\n --\tif ((len = xutftowcs_path(pattern, name)) < 0)\n -+\tif ((len = xutftowcs_path(pattern, name)) < 0 || len > MAX_PATH)\n + \tif ((len = xutftowcs_path(pattern, name)) < 0)\n \t\treturn NULL;\n \n ++\tif (len + 2 >= MAX_PATH)\n ++\t\treturn NULL;\n ++\n \t/* append optional '/' and wildcard '*' */\n + \tif (len && !is_dir_sep(pattern[len - 1]))\n + \t\tpattern[len++] = '/';\n\n\n compat/win32/dirent.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/compat/win32/dirent.c b/compat/win32/dirent.c\nindex 52420ec7d4d..fb63d1adbc5 100644\n--- a/compat/win32/dirent.c\n+++ b/compat/win32/dirent.c\n@@ -30,6 +30,9 @@ DIR *opendir(const char *name)\n \tif ((len = xutftowcs_path(pattern, name)) < 0)\n \t\treturn NULL;\n \n+\tif (len + 2 >= MAX_PATH)\n+\t\treturn NULL;\n+\n \t/* append optional '/' and wildcard '*' */\n \tif (len && !is_dir_sep(pattern[len - 1]))\n \t\tpattern[len++] = '/';\n\nbase-commit: 2ccc89b0c16c51561da90d21cfbb4b58cc877bf6\n-- \ngitgitgadget\n"}]}