[PATCH v11 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 . -- v11: 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. cmd: Don't attempt to copy a file to itself. 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 | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/programs/xcopy/xcopy.c b/programs/xcopy/xcopy.c index ee30e07b184..a8bd14f1616 100644 --- a/programs/xcopy/xcopy.c +++ b/programs/xcopy/xcopy.c @@ -345,7 +345,7 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, WCHAR *inputpath, *outputpath; BOOL copiedFile = FALSE; DWORD destAttribs, srcAttribs; - BOOL skipFile; + BOOL skipFile, quitCopy = FALSE; int ret = 0; /* Allocate some working memory on heap to minimize footprint */ @@ -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 && !quitCopy && findres) { skipFile = FALSE; @@ -550,11 +550,10 @@ 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; + quitCopy = TRUE; } } else { @@ -580,12 +579,14 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, } /* Find next file */ - findres = FindNextFileW(h, finddata); + if (!quitCopy) { + findres = FindNextFileW(h, finddata); + } } FindClose(h); /* Search 2 - do subdirs */ - if (flags & OPT_RECURSIVE) { + if (!quitCopy && (flags & OPT_RECURSIVE)) { /* If /E is supplied, create the directory now */ if ((flags & OPT_EMPTYDIR) && @@ -628,8 +629,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 | 48 ++++++++++++++++++++++++++++++++++++++--- programs/xcopy/xcopy.h | 1 + programs/xcopy/xcopy.rc | 1 + 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/programs/xcopy/xcopy.c b/programs/xcopy/xcopy.c index a8bd14f1616..09b8122c9d2 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, quitCopy = FALSE; + BOOL skipFile, quitCopy = FALSE, wantStatus = FALSE, sameFile = FALSE; int ret = 0; /* Allocate some working memory on heap to minimize footprint */ @@ -490,6 +521,12 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, } } + /* Don't copy a file over itself. */ + if (!skipFile && XCOPY_IsSameFile(copyFrom, copyTo)) { + skipFile = quitCopy = wantStatus = sameFile = TRUE; + ret = RC_INITERROR; + } + if (!skipFile && destAttribs != INVALID_FILE_ATTRIBUTES && !(flags & OPT_NOPROMPT)) { DWORD count; @@ -525,14 +562,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 Thu Nov 6 15:29:27 2025 +0000, Joe Souza wrote:
changed this line in [version 11 of the diff](/wine/wine/-/merge_requests/9112/diffs?diff_id=221691&start_sha=ae9e901728e4d91ee559afb30926308bdb7c1176#2a23d4e208dfe9e7509a35d1b5e209b2e69c0de5_82_82) Fixed
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121033
On Thu Nov 6 15:29:27 2025 +0000, Joe Souza wrote:
changed this line in [version 11 of the diff](/wine/wine/-/merge_requests/9112/diffs?diff_id=221691&start_sha=ae9e901728e4d91ee559afb30926308bdb7c1176#2ee4a78e992c1cb4cc1f3b7c24438b614ce2e6eb_410_410) Fixed
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121034
On Thu Nov 6 08:25:45 2025 +0000, eric pouech wrote:
perhaps a simple approach would be to detect if same file in the case of CopyFileW fails this would: * avoid adding extra variables that complexify the whole code flow, and add states that should be handled in various places... * avoid checking for the same file in successful copies (and spend unneeded time in it) CopyFileW returns ERROR_SHARING_VIOLATION in the case where a file is attempted to copy onto itself. How to differentiate between this and copying onto a different file that may be open by another process? That is the purpose of the IsSameFile function.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121035
On Thu Nov 6 08:26:17 2025 +0000, eric pouech wrote:
as already state in previous comments this can be done without adding an extraneous variable quitCopy is necessary for the same file case because it needs to execute the status display code later in the loop, which would be skipped if we instead just broke out of the loop.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121036
On Thu Nov 6 15:34:03 2025 +0000, Joe Souza wrote:
CopyFileW returns ERROR_SHARING_VIOLATION in the case where a file is attempted to copy onto itself. How to differentiate between this and copying onto a different file that may be open by another process? That is the purpose of the IsSameFile function. if mean something like:
``` if (!CopyFileW()) { if (GetLastError() == ERROR_SHARING_VIOLATION && IsSameFile()) {...} else {...} } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121136
On Thu Nov 6 15:36:23 2025 +0000, Joe Souza wrote:
quitCopy is necessary for the same file case because it needs to execute the status display code later in the loop, which would be skipped if we instead just broke out of the loop. and what about moving the test after the display?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121137
On Fri Nov 7 08:03:40 2025 +0000, eric pouech wrote:
if mean something like: ``` if (!CopyFileW()) { if (GetLastError() == ERROR_SHARING_VIOLATION && IsSameFile()) {...} else {...} } ``` The problem with moving IsSameFile() from where it is now to inside the error case for CopyFileW() is that native Windows does not prompt for overwrite if the file is the same. Therefore, IsSameFile() must be done before the prompting code, where it is now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121174
On Fri Nov 7 08:04:46 2025 +0000, eric pouech wrote:
and what about moving the test after the display? I will make changes to remove the quitCopy variable.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_121175
participants (3)
-
eric pouech (＠epo) -
Joe Souza
-
Joe Souza (＠JoeS209)