[PATCH v5 0/2] MR898: scrrun: Fix bugs in FileSystemObject text-file handling
Fix bugs in FileSystemObject text-file handling OpenTextFile(...,ForWriting,True) should either create a new file, or open and truncate an existing one OpenTextFile(...,ForAppending,?,True) should write a BOM if appending to an existing-but-empty file -- v5: scrrun: Fix FileSystemObject.OpenTextFile when writing to existing files. scrrun/tests: Introduce a test_file_contents helper. https://gitlab.winehq.org/wine/wine/-/merge_requests/898
From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com> --- dlls/scrrun/tests/filesystem.c | 97 ++++++++++++++-------------------- 1 file changed, 41 insertions(+), 56 deletions(-) diff --git a/dlls/scrrun/tests/filesystem.c b/dlls/scrrun/tests/filesystem.c index f2e23e4011c..41d7f8b5500 100644 --- a/dlls/scrrun/tests/filesystem.c +++ b/dlls/scrrun/tests/filesystem.c @@ -62,6 +62,32 @@ static inline void get_temp_path(const WCHAR *prefix, WCHAR *path) DeleteFileW(path); } +#define test_file_contents(a, b, c) _test_file_contents(a, b, c, __LINE__) +static void _test_file_contents(LPCWSTR pathW, DWORD expected_size, const void * expected_contents, int line) +{ + HANDLE file; + BOOL ret; + DWORD actual_size; + + file = CreateFileW(pathW, GENERIC_READ, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + ok_(__FILE__,line) (file != INVALID_HANDLE_VALUE, "got %p\n", file); + + actual_size = GetFileSize(file,NULL); + ok_(__FILE__,line) (actual_size == expected_size, "expected file %s size %ld, got %ld\n", wine_dbgstr_w(pathW), expected_size, actual_size); + + if((expected_contents != NULL) && (actual_size > 0)) + { + DWORD r; + void *actual_contents = malloc(actual_size); + ret = ReadFile(file, actual_contents, actual_size, &r, NULL); + ok_(__FILE__,line) (ret && r == actual_size, "Read %ld, got %d, error %ld.\n", r, ret, GetLastError()); + ok_(__FILE__,line) (!memcmp(expected_contents, actual_contents, expected_size), "expected file %s contents %s, got %s\n", wine_dbgstr_w(pathW), wine_dbgstr_an(expected_contents,expected_size), wine_dbgstr_an(actual_contents,r)); + free(actual_contents); + } + + CloseHandle(file); +} + static IDrive *get_fixed_drive(void) { IDriveCollection *drives; @@ -1507,13 +1533,12 @@ static void get_temp_filepath(const WCHAR *filename, WCHAR *path, WCHAR *dir) static void test_CreateTextFile(void) { - WCHAR pathW[MAX_PATH], dirW[MAX_PATH], buffW[10]; + WCHAR pathW[MAX_PATH], dirW[MAX_PATH]; ITextStream *stream; BSTR nameW, str; HANDLE file; HRESULT hr; BOOL ret; - DWORD r; get_temp_filepath(testfileW, pathW, dirW); @@ -1563,15 +1588,7 @@ static void test_CreateTextFile(void) ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); ITextStream_Release(stream); - /* check contents */ - file = CreateFileW(pathW, GENERIC_READ, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); - ok(file != INVALID_HANDLE_VALUE, "got %p\n", file); - r = 0; - ret = ReadFile(file, buffW, sizeof(buffW), &r, NULL); - ok(ret && r == 2, "Read %ld, got %d, error %ld.\n", r, ret, GetLastError()); - - ok( buffW[0] == 0xfeff, "got %04x\n", buffW[0] ); - CloseHandle(file); + test_file_contents(pathW,2,L"\ufeff"); DeleteFileW(nameW); RemoveDirectoryW(dirW); @@ -1580,14 +1597,13 @@ static void test_CreateTextFile(void) static void test_FolderCreateTextFile(void) { - WCHAR pathW[MAX_PATH], dirW[MAX_PATH], buffW[10], buff2W[10]; + WCHAR pathW[MAX_PATH], dirW[MAX_PATH]; ITextStream *stream; BSTR nameW, str; HANDLE file; IFolder *folder; HRESULT hr; BOOL ret; - DWORD r; get_temp_filepath(testfileW, pathW, dirW); nameW = SysAllocString(L"foo.txt"); @@ -1640,18 +1656,7 @@ static void test_FolderCreateTextFile(void) ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); ITextStream_Release(stream); - /* check contents */ - file = CreateFileW(pathW, GENERIC_READ, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); - ok(file != INVALID_HANDLE_VALUE, "got %p\n", file); - r = 0; - ret = ReadFile(file, buffW, sizeof(buffW), &r, NULL); - ok(ret && r == 2, "Read %ld, got %d, error %ld.\n", r, ret, GetLastError()); - buffW[r/sizeof(WCHAR)] = 0; - - buff2W[0] = 0xfeff; - buff2W[1] = 0; - ok(!lstrcmpW(buff2W, buffW), "got %s, expected %s\n", wine_dbgstr_w(buffW), wine_dbgstr_w(buff2W)); - CloseHandle(file); + test_file_contents(pathW,2,L"\ufeff"); DeleteFileW(pathW); RemoveDirectoryW(dirW); @@ -1661,11 +1666,10 @@ static void test_FolderCreateTextFile(void) static void test_WriteLine(void) { WCHAR pathW[MAX_PATH], dirW[MAX_PATH]; - WCHAR buffW[MAX_PATH], buff2W[MAX_PATH]; - char buffA[MAX_PATH]; + WCHAR ExpectedW[MAX_PATH]; + char ExpectedA[MAX_PATH]; ITextStream *stream; - DWORD r, len; - HANDLE file; + DWORD len; BSTR nameW; HRESULT hr; BOOL ret; @@ -1684,19 +1688,10 @@ static void test_WriteLine(void) ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); ITextStream_Release(stream); - /* check contents */ - file = CreateFileW(pathW, GENERIC_READ, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); - ok(file != INVALID_HANDLE_VALUE, "got %p\n", file); - r = 0; - ret = ReadFile(file, buffA, sizeof(buffA), &r, NULL); - ok(ret && r, "Read %ld, got %d, error %ld.\n", r, ret, GetLastError()); - - len = MultiByteToWideChar(CP_ACP, 0, buffA, r, buffW, ARRAY_SIZE(buffW)); - buffW[len] = 0; - lstrcpyW(buff2W, nameW); - lstrcatW(buff2W, L"\r\n"); - ok(!lstrcmpW(buff2W, buffW), "got %s, expected %s\n", wine_dbgstr_w(buffW), wine_dbgstr_w(buff2W)); - CloseHandle(file); + len = WideCharToMultiByte(CP_ACP, 0, nameW, -1, ExpectedA, ARRAY_SIZE(ExpectedA)-2, NULL, NULL); + ok(len > 0, "got %ld", len); + lstrcatA(ExpectedA, "\r\n"); + test_file_contents(pathW,lstrlenA(ExpectedA),ExpectedA); DeleteFileW(nameW); /* same for unicode file */ @@ -1707,20 +1702,10 @@ static void test_WriteLine(void) ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); ITextStream_Release(stream); - /* check contents */ - file = CreateFileW(pathW, GENERIC_READ, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); - ok(file != INVALID_HANDLE_VALUE, "got %p\n", file); - r = 0; - ret = ReadFile(file, buffW, sizeof(buffW), &r, NULL); - ok(ret && r, "Read %ld, got %d, error %ld.\n", r, ret, GetLastError()); - buffW[r/sizeof(WCHAR)] = 0; - - buff2W[0] = 0xfeff; - buff2W[1] = 0; - lstrcatW(buff2W, nameW); - lstrcatW(buff2W, L"\r\n"); - ok(!lstrcmpW(buff2W, buffW), "got %s, expected %s\n", wine_dbgstr_w(buffW), wine_dbgstr_w(buff2W)); - CloseHandle(file); + lstrcpyW(ExpectedW, L"\ufeff"); + lstrcatW(ExpectedW, nameW); + lstrcatW(ExpectedW, L"\r\n"); + test_file_contents(pathW,lstrlenW(ExpectedW)*sizeof(WCHAR),ExpectedW); DeleteFileW(nameW); RemoveDirectoryW(dirW); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/898
From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com> OpenTextFile(...,ForWriting,True) should either create a new file, or open and truncate an existing one OpenTextFile(...,ForAppending,?,True) should write a BOM if appending to an existing-but-empty file --- dlls/scrrun/filesystem.c | 23 ++++++++++++++++++++--- dlls/scrrun/tests/filesystem.c | 26 ++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/dlls/scrrun/filesystem.c b/dlls/scrrun/filesystem.c index cfeb5bb1532..e53355267db 100644 --- a/dlls/scrrun/filesystem.c +++ b/dlls/scrrun/filesystem.c @@ -897,7 +897,7 @@ static HRESULT create_textstream(const WCHAR *filename, DWORD disposition, IOMod { stream->unicode = format == TristateTrue; /* Write Unicode BOM */ - if (stream->unicode && (disposition == CREATE_ALWAYS || disposition == CREATE_NEW)) { + if (stream->unicode && (disposition == CREATE_ALWAYS || disposition == CREATE_NEW || disposition == TRUNCATE_EXISTING)) { DWORD written = 0; BOOL ret = WriteFile(stream->file, &utf16bom, sizeof(utf16bom), &written, NULL); if (!ret || written != sizeof(utf16bom)) { @@ -939,7 +939,20 @@ static HRESULT create_textstream(const WCHAR *filename, DWORD disposition, IOMod stream->eof = read != sizeof(buf); } - else SetFilePointer(stream->file, 0, 0, FILE_END); + else + { + LONG filePosHigh = 0; + DWORD filePosLow = SetFilePointer(stream->file, 0, &filePosHigh, FILE_END); + if(stream->unicode && filePosHigh == 0 && filePosLow == 0) { + /* unicode ForAppending to an empty file, write BOM */ + DWORD written = 0; + BOOL ret = WriteFile(stream->file, &utf16bom, sizeof(utf16bom), &written, NULL); + if (!ret || written != sizeof(utf16bom)) { + ITextStream_Release(&stream->ITextStream_iface); + return create_error(GetLastError()); + } + } + } } init_classinfo(&CLSID_TextStream, (IUnknown *)&stream->ITextStream_iface, &stream->classinfo); @@ -4049,7 +4062,11 @@ static HRESULT WINAPI filesys_OpenTextFile(IFileSystem3 *iface, BSTR filename, TRACE("(%p)->(%s %d %d %d %p)\n", iface, debugstr_w(filename), mode, create, format, stream); - disposition = create == VARIANT_TRUE ? OPEN_ALWAYS : OPEN_EXISTING; + if(mode == ForWriting) { + disposition = create == VARIANT_TRUE ? CREATE_ALWAYS : TRUNCATE_EXISTING; + } else { + disposition = create == VARIANT_TRUE ? OPEN_ALWAYS : OPEN_EXISTING; + } return create_textstream(filename, disposition, mode, format, stream); } diff --git a/dlls/scrrun/tests/filesystem.c b/dlls/scrrun/tests/filesystem.c index 41d7f8b5500..e53bd092ce9 100644 --- a/dlls/scrrun/tests/filesystem.c +++ b/dlls/scrrun/tests/filesystem.c @@ -263,6 +263,7 @@ static void test_createfolder(void) static void test_textstream(void) { + WCHAR ExpectedW[10]; ITextStream *stream; VARIANT_BOOL b; DWORD written; @@ -378,6 +379,31 @@ todo_wine { ok(ret && written == sizeof(testfileW), "got %d\n", ret); CloseHandle(file); + /* opening a non-empty file (from above) for writing should truncate it */ + hr = IFileSystem3_OpenTextFile(fs3, name, ForWriting, VARIANT_FALSE, TristateFalse, &stream); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ITextStream_Release(stream); + + test_file_contents(testfileW,0,""); + + /* appending to an empty file file and specifying unicode should immediately write a BOM */ + hr = IFileSystem3_OpenTextFile(fs3, name, ForAppending, VARIANT_FALSE, TristateTrue, &stream); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ITextStream_Release(stream); + + test_file_contents(testfileW,2,L"\ufeff"); + + /* appending to a file that contains a BOM should detect unicode mode, but not write a second BOM */ + hr = IFileSystem3_OpenTextFile(fs3, name, ForAppending, VARIANT_FALSE, TristateUseDefault, &stream); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = ITextStream_Write(stream, name); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ITextStream_Release(stream); + + lstrcpyW(ExpectedW, L"\ufeff"); + lstrcatW(ExpectedW, name); + test_file_contents(testfileW,lstrlenW(ExpectedW)*sizeof(WCHAR),ExpectedW); + hr = IFileSystem3_OpenTextFile(fs3, name, ForReading, VARIANT_FALSE, TristateFalse, &stream); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); b = 10; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/898
Here you go. I also extracted a helper for testing file contents/describing mismatches, since it was already awkwardly duplicated in several places. Hopefully that's OK. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/898#note_9104
Thanks. Looks good to me. However since I have no permission for approval, we have to wait for a developer. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/898#note_9113
participants (3)
-
Kevin Puetz -
Kevin Puetz (＠puetzk)
-
Robert Wilhelm (＠sloper42)