[PATCH v12 0/5] MR9112: XCOPY and COPY should not attempt to copy a file to itself.
Current code attempts to copy a file to itself. File is not actually copied; instead, a file sharing error is received. This result is ugly and differs from native. Behavior now appears to be the same as native with changes in this MR . -- v12: xcopy: Don't attempt to copy a file to itself. xcopy/tests: Add test for XCOPY to self. xcopy: Fix leaked resource in an error case. https://gitlab.winehq.org/wine/wine/-/merge_requests/9112
From: Joe Souza <jsouza(a)yahoo.com> --- programs/cmd/tests/test_builtins.bat | 6 ++++++ programs/cmd/tests/test_builtins.bat.exp | 6 ++++++ programs/cmd/tests/test_builtins.cmd | 6 ++++++ programs/cmd/tests/test_builtins.cmd.exp | 6 ++++++ 4 files changed, 24 insertions(+) diff --git a/programs/cmd/tests/test_builtins.bat b/programs/cmd/tests/test_builtins.bat index dc624111213..e36cc742053 100644 --- a/programs/cmd/tests/test_builtins.bat +++ b/programs/cmd/tests/test_builtins.bat @@ -114,6 +114,12 @@ call :setError 666 & (copy fileA fileZ /-Y >NUL <NUL &&echo SUCCESS !errorlevel! call :setError 666 & (copy fileA+fileD fileZ /-Y >NUL <NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (copy fileD+fileA fileZ /-Y >NUL <NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) if exist fileD echo Unexpected fileD +call :setError 666 & (copy /b fileA fileA /Y >NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +call :setError 666 & (copy /b fileA+fileB fileA >NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +type fileA +echo a > fileA +call :setError 666 & (copy /b fileA+fileB /Y fileB >NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +type fileB cd .. && rd /q /s foo echo --- success/failure for MOVE command diff --git a/programs/cmd/tests/test_builtins.bat.exp b/programs/cmd/tests/test_builtins.bat.exp index d3f23abde77..6dbd418a8f5 100644 --- a/programs/cmd/tests/test_builtins.bat.exp +++ b/programs/cmd/tests/test_builtins.bat.exp @@ -77,6 +77,12 @@ SUCCESS 0 FAILURE 1 FAILURE 1 FAILURE 1 +FAILURE 1 +SUCCESS 0 +a +b +SUCCESS 0 +a --- success/failure for MOVE command FAILURE 1 SUCCESS 0 diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 19dc3c788bb..11d92dd16c4 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -639,6 +639,12 @@ call :setError 666 & (copy fileA fileZ /-Y >NUL <NUL &&echo SUCCESS !errorlevel! call :setError 666 & (copy fileA+fileD fileZ /-Y >NUL <NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (copy fileD+fileA fileZ /-Y >NUL <NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) if exist fileD echo Unexpected fileD +call :setError 666 & (copy /b fileA fileA /Y >NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +call :setError 666 & (copy /b fileA+fileB fileA >NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +type fileA +echo a > fileA +call :setError 666 & (copy /b fileA+fileB /Y fileB >NUL &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) +type fileB cd .. && rd /q /s foo echo --- success/failure for MOVE command diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 644ca910c8c..38bb41e181f 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -541,6 +541,12 @@ SUCCESS 0 FAILURE 1 FAILURE 1 FAILURE 1 +FAILURE 1 +SUCCESS 0 +a +b +SUCCESS 0 +a --- success/failure for MOVE command FAILURE 1 SUCCESS 0 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9112
From: Joe Souza <jsouza(a)yahoo.com> --- programs/cmd/builtins.c | 19 ++++++++++++++++--- programs/cmd/cmd.rc | 1 + programs/cmd/wcmd.h | 1 + 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index ef84bee64ac..f9a286dc92a 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -672,6 +672,7 @@ RETURN_CODE WCMD_copy(WCHAR * args) DWORD len; BOOL dstisdevice = FALSE; unsigned numcopied = 0; + BOOL want_numcopied = FALSE; typedef struct _COPY_FILES { @@ -1041,6 +1042,7 @@ RETURN_CODE WCMD_copy(WCHAR * args) WCHAR outname[MAX_PATH]; BOOL overwrite; BOOL appendtofirstfile = FALSE; + BOOL issamefile; /* Skip . and .., and directories */ if (!srcisdevice && fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { @@ -1060,13 +1062,24 @@ RETURN_CODE WCMD_copy(WCHAR * args) overwrite = TRUE; } + issamefile = WCMD_IsSameFile(srcpath, outname); + WINE_TRACE("Copying from : '%s'\n", wine_dbgstr_w(srcpath)); WINE_TRACE("Copying to : '%s'\n", wine_dbgstr_w(outname)); WINE_TRACE("Flags: srcbinary(%d), dstbinary(%d), over(%d), prompt(%d)\n", thiscopy->binarycopy, destination->binarycopy, overwrite, prompt); + if (!anyconcats && issamefile) { + WCMD_output_asis(srcpath); + WCMD_output_asis(L"\r\n"); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_NOCOPYTOSELF)); + return_code = ERROR_INVALID_FUNCTION; + want_numcopied = TRUE; + break; + } + if (!writtenoneconcat) { - appendtofirstfile = anyconcats && WCMD_IsSameFile(srcpath, outname); + appendtofirstfile = anyconcats && issamefile; } /* Prompt before overwriting */ @@ -1098,7 +1111,7 @@ RETURN_CODE WCMD_copy(WCHAR * args) WCMD_output_asis(srcpath); WCMD_output_asis(L"\r\n"); } - if (anyconcats && WCMD_IsSameFile(srcpath, outname)) { + if (anyconcats && issamefile) { /* behavior is as Unix 'touch' (change last-written time only) */ HANDLE file = CreateFileW(srcpath, GENERIC_WRITE, FILE_SHARE_WRITE, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); @@ -1168,7 +1181,7 @@ RETURN_CODE WCMD_copy(WCHAR * args) } } - if (numcopied) { + if (numcopied || want_numcopied) { WCMD_output(WCMD_LoadMessage(WCMD_NUMCOPIED), numcopied); } diff --git a/programs/cmd/cmd.rc b/programs/cmd/cmd.rc index 29d105fa7a2..bc738faa5e9 100644 --- a/programs/cmd/cmd.rc +++ b/programs/cmd/cmd.rc @@ -407,4 +407,5 @@ Enter HELP <command> for further information on any of the above commands.\n" WCMD_ENDOFLINE, "End of line" WCMD_ENDOFFILE, "End of file" WCMD_NUMCOPIED, "%t%1!u! file(s) copied\n" + WCMD_NOCOPYTOSELF, "File cannot be copied onto itself.\n" } diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index 6810598142c..ba18b942c5d 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -477,3 +477,4 @@ extern WCHAR version_string[]; #define WCMD_ENDOFLINE 1048 #define WCMD_ENDOFFILE 1049 #define WCMD_NUMCOPIED 1050 +#define WCMD_NOCOPYTOSELF 1051 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9112
From: Joe Souza <jsouza(a)yahoo.com> --- programs/xcopy/xcopy.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/programs/xcopy/xcopy.c b/programs/xcopy/xcopy.c index ee30e07b184..77518fc56b2 100644 --- a/programs/xcopy/xcopy.c +++ b/programs/xcopy/xcopy.c @@ -359,7 +359,7 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, /* Search 1 - Look for matching files */ h = FindFirstFileW(inputpath, finddata); - while (h != INVALID_HANDLE_VALUE && findres) { + while (h != INVALID_HANDLE_VALUE && !ret && findres) { skipFile = FALSE; @@ -550,11 +550,9 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, copyFrom, copyTo, error); XCOPY_FailMessage(error); - if (flags & OPT_IGNOREERRORS) { - skipFile = TRUE; - } else { + skipFile = TRUE; + if (!(flags & OPT_IGNOREERRORS)) { ret = RC_WRITEERROR; - goto cleanup; } } else { @@ -580,12 +578,14 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, } /* Find next file */ - findres = FindNextFileW(h, finddata); + if (!ret) { + findres = FindNextFileW(h, finddata); + } } FindClose(h); /* Search 2 - do subdirs */ - if (flags & OPT_RECURSIVE) { + if (!ret && (flags & OPT_RECURSIVE)) { /* If /E is supplied, create the directory now */ if ((flags & OPT_EMPTYDIR) && @@ -628,8 +628,6 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, FindClose(h); } -cleanup: - /* free up memory */ HeapFree(GetProcessHeap(), 0, finddata); HeapFree(GetProcessHeap(), 0, inputpath); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9112
From: Joe Souza <jsouza(a)yahoo.com> --- programs/xcopy/tests/xcopy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/programs/xcopy/tests/xcopy.c b/programs/xcopy/tests/xcopy.c index e038f69ad02..565bd11af00 100644 --- a/programs/xcopy/tests/xcopy.c +++ b/programs/xcopy/tests/xcopy.c @@ -124,6 +124,9 @@ static void test_parms_syntax(void) ok(GetFileAttributesA("xcopy test2") != INVALID_FILE_ATTRIBUTES, "xcopy failed to copy empty directory\n"); RemoveDirectoryA("xcopy test2"); + + rc = runcmd("xcopy xcopy1 xcopy1"); + ok(rc == 4, "xcopy file to self failed rc=%lu\n", rc); } static void test_keep_attributes(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9112
From: Joe Souza <jsouza(a)yahoo.com> --- programs/xcopy/xcopy.c | 50 ++++++++++++++++++++++++++++++++++++++--- programs/xcopy/xcopy.h | 1 + programs/xcopy/xcopy.rc | 1 + 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/programs/xcopy/xcopy.c b/programs/xcopy/xcopy.c index 77518fc56b2..02004ceac1c 100644 --- a/programs/xcopy/xcopy.c +++ b/programs/xcopy/xcopy.c @@ -325,6 +325,37 @@ static BOOL XCOPY_ProcessExcludeList(WCHAR* parms) { return FALSE; } +/* ========================================================================= + * XCOPY_IsSameFile + * + * Checks if the two paths reference to the same file. + * Copied from WCMD builtins.c, and tab-adjusted. + * ========================================================================= */ +static BOOL XCOPY_IsSameFile(const WCHAR *name1, const WCHAR *name2) +{ + BOOL ret = FALSE; + HANDLE file1 = INVALID_HANDLE_VALUE, file2 = INVALID_HANDLE_VALUE; + BY_HANDLE_FILE_INFORMATION info1, info2; + + file1 = CreateFileW(name1, 0, FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, 0, OPEN_EXISTING, 0, 0); + if (file1 == INVALID_HANDLE_VALUE || !GetFileInformationByHandle(file1, &info1)) + goto end; + + file2 = CreateFileW(name2, 0, FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, 0, OPEN_EXISTING, 0, 0); + if (file2 == INVALID_HANDLE_VALUE || !GetFileInformationByHandle(file2, &info2)) + goto end; + + ret = info1.dwVolumeSerialNumber == info2.dwVolumeSerialNumber + && info1.nFileIndexHigh == info2.nFileIndexHigh + && info1.nFileIndexLow == info2.nFileIndexLow; +end: + if (file1 != INVALID_HANDLE_VALUE) + CloseHandle(file1); + if (file2 != INVALID_HANDLE_VALUE) + CloseHandle(file2); + return ret; +} + /* ========================================================================= XCOPY_DoCopy - Recursive function to copy files based on input parms of a stem and a spec @@ -345,7 +376,7 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, WCHAR *inputpath, *outputpath; BOOL copiedFile = FALSE; DWORD destAttribs, srcAttribs; - BOOL skipFile; + BOOL skipFile, wantStatus = FALSE, sameFile = FALSE; int ret = 0; /* Allocate some working memory on heap to minimize footprint */ @@ -490,6 +521,14 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, } } + /* Don't copy a file over itself. This check must be performed before the + overwrite prompt occurs, because native Windows does not prompt for + overwrite if the file is the same. */ + if (!skipFile && XCOPY_IsSameFile(copyFrom, copyTo)) { + skipFile = wantStatus = sameFile = TRUE; + ret = RC_INITERROR; + } + if (!skipFile && destAttribs != INVALID_FILE_ATTRIBUTES && !(flags & OPT_NOPROMPT)) { DWORD count; @@ -525,14 +564,19 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, } /* Output a status message */ - if (!skipFile) { - if (!(flags & OPT_QUIET)) { + if (!skipFile || wantStatus) { + if (!(flags & OPT_QUIET) && !(flags & OPT_SRCPROMPT)) { if (flags & OPT_FULL) XCOPY_wprintf(L"%1 -> %2\n", copyFrom, copyTo); else XCOPY_wprintf(L"%1\n", copyFrom); } + if (sameFile) { + XCOPY_wprintf(XCOPY_LoadMessage(STRING_NOCOPYTOSELF)); + } + } + if (!skipFile) { /* If allowing overwriting of read only files, remove any write protection */ if ((destAttribs & FILE_ATTRIBUTE_READONLY) && diff --git a/programs/xcopy/xcopy.h b/programs/xcopy/xcopy.h index 1ed9e20c45b..70b0935ba2c 100644 --- a/programs/xcopy/xcopy.h +++ b/programs/xcopy/xcopy.h @@ -69,3 +69,4 @@ #define STRING_FILE_CHAR 115 #define STRING_DIR_CHAR 116 #define STRING_HELP 117 +#define STRING_NOCOPYTOSELF 118 diff --git a/programs/xcopy/xcopy.rc b/programs/xcopy/xcopy.rc index c61e4da88fd..ae276c6b876 100644 --- a/programs/xcopy/xcopy.rc +++ b/programs/xcopy/xcopy.rc @@ -79,4 +79,5 @@ Where:\n\ \t\tIf no date is supplied, only copy if destination is older\n\ \t\tthan source.\n\n" + STRING_NOCOPYTOSELF, "File cannot be copied onto itself.\n" } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9112
On Fri Nov 7 17:57:01 2025 +0000, Joe Souza wrote:
Fixed I... don't think that's better?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121197
On Fri Nov 7 16:01:45 2025 +0000, Joe Souza wrote:
changed this line in [version 12 of the diff](/wine/wine/-/merge_requests/9112/diffs?diff_id=221996&start_sha=d1b2fb007749aaf28c26e9a3349a1561bd9b1799#50aa41b98a0279d229f39762936025608dbac8ab_524_524) you could skip printing the prompt is IsSameFile is TRUE, and output the error message if CopyFileW fails (sharing violation + another call to is same file
yes that would make two calls to issamefile in the case of xcopy foo foo, but this makes the code simpler to read, and only overloads the error path in that very case we would need a tristate variable in we wanted to call issamefile only once, but I'm not sure that's worth the effort please also get rid of the label & goto in issamefile helper -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121488
participants (4)
-
Elizabeth Figura (＠zfigura) -
eric pouech (＠epo)
-
Joe Souza
-
Joe Souza (＠JoeS209)