[PATCH v13 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 . -- v13: 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 | 37 ++++++++++++++++++++++++------------- programs/cmd/cmd.rc | 1 + programs/cmd/wcmd.h | 1 + 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index ef84bee64ac..4371eff97c2 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -522,17 +522,15 @@ static BOOL WCMD_IsSameFile(const WCHAR *name1, const WCHAR *name2) 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; + if (file1 != INVALID_HANDLE_VALUE && GetFileInformationByHandle(file1, &info1)) { + 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)) { + ret = info1.dwVolumeSerialNumber == info2.dwVolumeSerialNumber + && info1.nFileIndexHigh == info2.nFileIndexHigh + && info1.nFileIndexLow == info2.nFileIndexLow; + } + } - 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) @@ -672,6 +670,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 +1040,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 +1060,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 +1109,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 +1179,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 | 56 +++++++++++++++++++++++++++++++++-------- programs/xcopy/xcopy.h | 1 + programs/xcopy/xcopy.rc | 1 + 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/programs/xcopy/xcopy.c b/programs/xcopy/xcopy.c index 77518fc56b2..426c7f4c660 100644 --- a/programs/xcopy/xcopy.c +++ b/programs/xcopy/xcopy.c @@ -325,6 +325,35 @@ 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)) { + 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)) { + ret = info1.dwVolumeSerialNumber == info2.dwVolumeSerialNumber + && info1.nFileIndexHigh == info2.nFileIndexHigh + && info1.nFileIndexLow == info2.nFileIndexLow; + } + } + + 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 @@ -491,7 +520,9 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, } if (!skipFile && - destAttribs != INVALID_FILE_ATTRIBUTES && !(flags & OPT_NOPROMPT)) { + destAttribs != INVALID_FILE_ATTRIBUTES && !(flags & OPT_NOPROMPT) && + !XCOPY_IsSameFile(copyFrom, copyTo)) { + DWORD count; char answer[10]; BOOL answered = FALSE; @@ -526,13 +557,15 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, /* Output a status message */ if (!skipFile) { - if (!(flags & OPT_QUIET)) { + 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 (!skipFile) { /* If allowing overwriting of read only files, remove any write protection */ if ((destAttribs & FILE_ATTRIBUTE_READONLY) && @@ -544,18 +577,21 @@ static int XCOPY_DoCopy(WCHAR *srcstem, WCHAR *srcspec, if (flags & OPT_SIMULATE || flags & OPT_NOCOPY) { /* Skip copy */ } else if (CopyFileW(copyFrom, copyTo, FALSE) == 0) { - - DWORD error = GetLastError(); - XCOPY_wprintf(XCOPY_LoadMessage(STRING_COPYFAIL), - copyFrom, copyTo, error); - XCOPY_FailMessage(error); - skipFile = TRUE; - if (!(flags & OPT_IGNOREERRORS)) { + if (XCOPY_IsSameFile(copyFrom, copyTo)) { + XCOPY_wprintf(XCOPY_LoadMessage(STRING_NOCOPYTOSELF)); ret = RC_WRITEERROR; + } else { + DWORD error = GetLastError(); + XCOPY_wprintf(XCOPY_LoadMessage(STRING_COPYFAIL), + copyFrom, copyTo, error); + XCOPY_FailMessage(error); + + if (!(flags & OPT_IGNOREERRORS)) { + ret = RC_WRITEERROR; + } } } else { - if (!skipFile) { /* If keeping attributes, update the destination attributes otherwise remove the read only attribute */ 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
participants (2)
-
Joe Souza -
Joe Souza (＠JoeS209)