[PATCH 0/2] 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 . -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112
From: Joe Souza <jsouza(a)yahoo.com> --- programs/xcopy/xcopy.c | 62 ++++++++++++++++++++++++++++++++++------- programs/xcopy/xcopy.h | 1 + programs/xcopy/xcopy.rc | 1 + 3 files changed, 54 insertions(+), 10 deletions(-) diff --git a/programs/xcopy/xcopy.c b/programs/xcopy/xcopy.c index ee30e07b184..8bb4238a8f1 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. + * ========================================================================= */ +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, quitCopy = FALSE; int ret = 0; /* Allocate some working memory on heap to minimize footprint */ @@ -359,7 +390,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; @@ -465,6 +496,18 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, } } + /* Don't copy a file over itself. */ + if (XCOPY_IsSameFile(copyFrom, copyTo)) { + skipFile = quitCopy = TRUE; + if (!(flags & OPT_QUIET)) { + if (flags & OPT_FULL) + XCOPY_wprintf(L"%1 -> %2\n", copyFrom, copyTo); + else + XCOPY_wprintf(L"%1\n", copyFrom); + } + XCOPY_wprintf(XCOPY_LoadMessage(STRING_NOCOPYTOSELF)); + } + /* Prompt each file if necessary */ if (!skipFile && (flags & OPT_SRCPROMPT)) { DWORD count; @@ -550,11 +593,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 +622,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 +672,6 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, FindClose(h); } -cleanup: - /* free up memory */ HeapFree(GetProcessHeap(), 0, finddata); HeapFree(GetProcessHeap(), 0, inputpath); 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..b12eeb4b068 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 to itself.\n" } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9112
From: Joe Souza <jsouza(a)yahoo.com> --- programs/cmd/builtins.c | 20 ++++++++++++++++---- programs/cmd/cmd.rc | 1 + programs/cmd/wcmd.h | 1 + 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index ef84bee64ac..f556b673382 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -1041,6 +1041,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 +1061,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); + WINE_TRACE("Flags: srcbinary(%d), dstbinary(%d), over(%d), prompt(%d), issamefile(%d)\n", + thiscopy->binarycopy, destination->binarycopy, overwrite, prompt, issamefile); + + if (!anyconcats && issamefile) { + WCMD_output_asis(srcpath); + WCMD_output_asis(L"\r\n"); + WCMD_output_stderr(WCMD_LoadMessage(WCMD_NOCOPYTOSELF)); + WCMD_output(WCMD_LoadMessage(WCMD_NUMCOPIED), numcopied); + return_code = ERROR_INVALID_FUNCTION; + goto exitreturn; + } if (!writtenoneconcat) { - appendtofirstfile = anyconcats && WCMD_IsSameFile(srcpath, outname); + appendtofirstfile = anyconcats && issamefile; } /* Prompt before overwriting */ @@ -1098,7 +1110,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); diff --git a/programs/cmd/cmd.rc b/programs/cmd/cmd.rc index 29d105fa7a2..322673aab30 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 to 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
(cursory look, this isn't a full review yet) please add tests for these UC, covering single and multiple files copy (ie copy a a; copy a+b a; copy a+b b --- from a quick look in native, the three caseq have different behaviors) IIRC, in the third case using 'copy .. < NUL' forces the 'Y' case (using echo Y|copy... doesn't work in all locales) for COPY, it should be possible to fold the error path into the existing code instead of stuffing into a new block of codes a bunch of existing lines of code adding the same-file to the traces is useless IMO for XCOPY, stick to the existing style of the file (ie indentation is wrong) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_117760
On Wed Oct 8 17:01:29 2025 +0000, eric pouech wrote:
(cursory look, this isn't a full review yet) please add tests for these UC, covering single and multiple files copy (ie copy a a; copy a+b a; copy a+b b --- from a quick look in native, the three caseq have different behaviors) IIRC, in the third case using 'copy .. < NUL' forces the 'Y' case (using echo Y|copy... doesn't work in all locales) for COPY, it should be possible to fold the error path into the existing code instead of stuffing into a new block of codes a bunch of existing lines of code adding the same-file to the traces is useless IMO for XCOPY, stick to the existing style of the file (ie indentation is wrong) The three cases you list here seem to work correctly with my changes. One difference, however, is <NUL does not work for the third case. Receive a "File not found." message. This seems like a separate bug in the existing code.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_118011
On Wed Oct 8 17:01:29 2025 +0000, Joe Souza wrote:
The three cases you list here seem to work correctly with my changes. One difference, however, is <NUL does not work for the third case. Receive a "File not found." message. This seems like a separate bug in the existing code. Just found that ```< NUL``` works (i.e. space between < and NUL). ```<NUL``` with no space does not. Will leave that as a separate bug outside the scope of the changes here.
Still working on other changes you requested. Not quite sure yet what to do for the tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_118012
for the tests, I think we may want: for COPY, in programs/cmd/tests, in the section (--- success/failure for COPY command) both in the test_builtins.bat and .cmd cases (*), add (with the same wrappers as other tests): - COPY fileA fileA - COPY fileA+fileB fileA and check fileA contents after COPY (maybe reset it if content has been altered) - COPY fileA+fileB fileB and check fileB contents after COPY (maybe reset it if content has been altered) Note: the 'COPY fileA+' (not listed above) case is hard to test, so it rather be tested by hand for now (fileA is not erased, only the modification time is changed) (*) we need both as some return codes may vary between .cmd and .bat for XCOPY, since it's an external command, it should rather be done in programs/xcopy, but same three cases as above is a good start (+ checking exit codes) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_118273
participants (3)
-
eric pouech (＠epo) -
Joe Souza
-
Joe Souza (＠JoeS209)