[PATCH v2 0/1] MR8352: xcopy: Fix out-of-bound access when parsing arguments.

[Yuxuan Shui (＠yshui)]

We didn't check if the argument string is long enough before indexing into it.

--
  v2: xcopy: Fix out-of-bound access when parsing arguments.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8352

[PATCH v2 1/1] xcopy: Fix out-of-bound access when parsing arguments.

[Yuxuan Shui]

From: Yuxuan Shui <yshui(a)codeweavers.com>

We didn't check if the argument string is long enough before indexing into it.
---
 programs/xcopy/xcopy.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/programs/xcopy/xcopy.c b/programs/xcopy/xcopy.c
index ce3e40d7d5e..5b837b70edb 100644
--- a/programs/xcopy/xcopy.c
+++ b/programs/xcopy/xcopy.c
@@ -737,9 +737,9 @@ static int XCOPY_ParseCommandLine(WCHAR *suppliedsource,
                  Note: Windows docs say /P prompts when dest is created
                        but tests show it is done for each src file
                        regardless of the destination                   */
-            WCHAR *p = word + 1, *rest;
+            WCHAR *p = word + 1, *rest, *end = p + wcslen(p);
 
-            while (*p) {
+            while (p != end) {
                 rest = p + 1;
 
                 switch (toupper(*p)) {
@@ -763,14 +763,15 @@ static int XCOPY_ParseCommandLine(WCHAR *suppliedsource,
                                    OPT_REMOVEARCH;    break;
 
                 /* E can be /E or /EXCLUDE */
-                case 'E': if (CompareStringW(LOCALE_USER_DEFAULT, NORM_IGNORECASE | SORT_STRINGSORT,
+                case 'E': if (p+8<=end &&
+                              CompareStringW(LOCALE_USER_DEFAULT, NORM_IGNORECASE | SORT_STRINGSORT,
                                              p, 8, L"EXCLUDE:", -1) == CSTR_EQUAL) {
                             if (XCOPY_ProcessExcludeList(&p[8])) {
                               XCOPY_FailMessage(ERROR_INVALID_PARAMETER);
                               exit(RC_INITERROR);
                             } else {
                               flags |= OPT_EXCLUDELIST;
-                              rest = p + wcslen(p);
+                              rest = end;
                             }
                           } else {
                               flags |= OPT_EMPTYDIR | OPT_RECURSIVE;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8352

[Alexandre Julliard (＠julliard)]

On Wed Jun 18 14:07:11 2025 +0000, Yuxuan Shui wrote:
> oh, i get it now, you are saying if `p[1]==0` we won't use `p[2]`.
> Yes, checking characters one by one is a common pattern, and it's safe as long as it's done in the right order.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8352#note_107060