[PATCH 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.

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

[PATCH 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 | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/programs/xcopy/xcopy.c b/programs/xcopy/xcopy.c
index ce3e40d7d5e..2ea5da00c0d 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;
@@ -778,7 +779,7 @@ static int XCOPY_ParseCommandLine(WCHAR *suppliedsource,
                           break;
 
                 /* D can be /D or /D: */
-                case 'D': if (p[1]==':' && is_digit(p[2])) {
+                case 'D': if (p+3<=end && p[1]==':' && is_digit(p[2])) {
                               SYSTEMTIME st;
                               WCHAR     *pos = &p[2];
                               BOOL       isError = FALSE;
@@ -831,7 +832,7 @@ static int XCOPY_ParseCommandLine(WCHAR *suppliedsource,
                           }
                           break;
 
-                case '-': if (toupper(p[1])=='Y') {
+                case '-': if (p+2<=end && toupper(p[1])=='Y') {
                               flags &= ~OPT_NOPROMPT;
                               rest = &p[2];  /* Skip over 2 characters */
                           }
-- 
GitLab

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

[Alexandre Julliard (＠julliard)]

Alexandre Julliard (@julliard) commented about programs/xcopy/xcopy.c:
> break;
>
> /* D can be /D or /D: */
> -                case 'D': if (p[1]==':' && is_digit(p[2])) {
> +                case 'D': if (p+3<=end && p[1]==':' && is_digit(p[2])) {
>                                SYSTEMTIME st;

That doesn't seem necessary.

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

[Yuxuan Shui (＠yshui)]

On Wed Jun 18 09:24:55 2025 +0000, Alexandre Julliard wrote:
> That doesn't seem necessary.
> is `p` guaranteed to be at least 3 characters long?

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

[Alexandre Julliard (＠julliard)]

On Wed Jun 18 13:16:15 2025 +0000, Yuxuan Shui wrote:
> is `p` guaranteed to be at least 3 characters long?
> Yes, if p[0] == 'D', p[1] == ':' and p[2] is a digit then it has to be at least 3 characters long.

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

[Yuxuan Shui (＠yshui)]

On Wed Jun 18 13:48:21 2025 +0000, Alexandre Julliard wrote:
> Yes, if p[0] == 'D', p[1] == ':' and p[2] is a digit then it has to be
> at least 3 characters long.
> but you can't use `p[1]` and `p[2]` unless you already know it's 3 characters long?

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

[Nikolay Sivov (＠nsivov)]

On Wed Jun 18 13:49:14 2025 +0000, Yuxuan Shui wrote:
> but you can't use `p[1]` and `p[2]` unless you already know it's 3
> characters long?
> Is it not null-terminated?

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

[Yuxuan Shui (＠yshui)]

On Wed Jun 18 13:52:11 2025 +0000, Nikolay Sivov wrote:
> Is it not null-terminated?
> oh, i get it now, you are saying if `p[1]==0` we won't use `p[2]`.

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