[PATCH v10 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 . -- v10: xcopy: 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..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
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..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
Latest changes pushed, ready for merging or further review. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_120742
eric pouech (@epo) commented about programs/xcopy/xcopy.rc:
\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" onto
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_120993
eric pouech (@epo) commented about programs/xcopy/xcopy.c:
} }
+ /* Don't copy a file over itself. */
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) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_120994
eric pouech (@epo) commented about programs/cmd/cmd.rc:
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" onto
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_120995
eric pouech (@epo) commented about programs/xcopy/xcopy.c:
WCHAR *inputpath, *outputpath; BOOL copiedFile = FALSE; DWORD destAttribs, srcAttribs; - BOOL skipFile; + BOOL skipFile, quitCopy = FALSE;
as already state in previous comments this can be done without adding an extraneous variable -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9112#note_120996
participants (3)
-
eric pouech (＠epo) -
Joe Souza
-
Joe Souza (＠JoeS209)