I stumbled onto this while using freopen() for debugging purposes. Basically, freopen() fails if the FILE has been created with an invalid handle.
So, this MR contains: - basic tests for freopen (no issue there, just for coverage purposes) - tests for freopen on FILE with invalid handle - fix for freopen
-- v2: msvcrt: Fix freopen() on FILE with handle=-2. isfd msvcrt/tests: Add tests for freopen().
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/tests/file.c | 68 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)
diff --git a/dlls/msvcrt/tests/file.c b/dlls/msvcrt/tests/file.c index 3f57b3f871f..2f3e6e00d0b 100644 --- a/dlls/msvcrt/tests/file.c +++ b/dlls/msvcrt/tests/file.c @@ -1167,6 +1167,73 @@ static void test_fputwc(void) _unlink(tempfile); }
+static void test_freopen( void ) +{ + char filename1[8] = "AXXXXXX"; + char filename2[8] = "BXXXXXX"; + FILE *file; + int ret; + int fd; + char ch; + long pos; + + mktemp(filename1); + mktemp(filename2); + + file = fopen(filename1, "wt"); + ok(file != NULL, "fopen(filename1) returned NULL\n"); + ret = fwrite("1", 1, 1, file); + ok(ret == 1, "fwrite() returned %d (%d)\n", ret, errno); + ret = fclose(file); + ok(ret == 0, "fclose() returned %d\n", ret); + + file = fopen(filename2, "wt"); + ok(file != NULL, "fopen(filename1) returned NULL\n"); + ret = fwrite("2", 1, 1, file); + ok(ret == 1, "fwrite() returned %d (%d)\n", ret, errno); + ret = fclose(file); + ok(ret == 0, "fclose() returned %d\n", ret); + + file = fopen(filename1, "rt"); + ok(file != NULL, "fopen(filename1) returned NULL\n"); + file = freopen(filename2, "rt", file); + ok(file != NULL, "fopen(filename2) returned NULL\n"); + ch = '#'; + ret = fread(&ch, 1, 1, file); + ok(ret == 1, "fread() returned %d\n", ret); + ok(ch == '2', "fread() read %c\n", ch); + ret = fclose(file); + ok(ret == 0, "fclose() returned %d\n", ret); + + file = fopen(filename1, "at"); + ok(file != NULL, "fopen(filename1) returned NULL\n"); + file = freopen(filename1, "rt", file); + ok(file != NULL, "fopen(filename1) returned NULL\n"); + pos = ftell(file); + ok(pos == 0, "ftell() returned %ld\n", pos); + ch = '#'; + ret = fread(&ch, 1, 1, file); + ok(ret == 1, "fread() returned %d\n", ret); + ok(ch == '1', "fread() read %c\n", ch); + ret = fclose(file); + ok(ret == 0, "fclose() returned %d\n", ret); + + file = fopen(filename1, "rt"); + ok(file != NULL, "fopen(filename1) returned NULL\n"); + fd = fileno(file); + ok(fd > 0, "fileno() returned %d\n", fd); + file = freopen("_:", "rt", file); + ok(file == NULL, "fopen(_:) returned non NULL\n"); + errno = 0xdeadbeef; + ch = '#'; + ret = read(fd, &ch, 1); + ok(ret == -1, "read() returned %d\n", ret); + ok(errno == EBADF, "errno is %d\n", errno); + + unlink(filename1); + unlink(filename2); +} + static void test_ctrlz( void ) { char* tempf; @@ -2977,6 +3044,7 @@ START_TEST(file) test_fgetwc_locale("AB\x83\xa9", "C", 0); test_fgetwc_unicode(); test_fputwc(); + test_freopen(); test_ctrlz(); test_file_put_get(); test_tmpnam();
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/tests/file.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/dlls/msvcrt/tests/file.c b/dlls/msvcrt/tests/file.c index 2f3e6e00d0b..106c1554518 100644 --- a/dlls/msvcrt/tests/file.c +++ b/dlls/msvcrt/tests/file.c @@ -1172,6 +1172,7 @@ static void test_freopen( void ) char filename1[8] = "AXXXXXX"; char filename2[8] = "BXXXXXX"; FILE *file; + FILE *new; int ret; int fd; char ch; @@ -1230,6 +1231,36 @@ static void test_freopen( void ) ok(ret == -1, "read() returned %d\n", ret); ok(errno == EBADF, "errno is %d\n", errno);
+ file = fopen(filename1, "rb"); + ok(file != NULL, "couldn't open %s\n", filename1); + close(file->_file); + file->_file = -1; + + new = freopen(filename2, "rb", file); + todo_wine + ok(new == file, "freopen() didn't return same FILE*\n"); + if (new) { /* temporary as it crashes on wine */ + + fd = fileno(new); + todo_wine + ok(fd > 0, "fileno() returned %d\n", fd); + + errno = 0xdeadbeef; + ch = '#'; + ret = fread(&ch, 1, 1, new); + todo_wine + ok(ret == 1, "fread() returned %d\n", ret); + todo_wine + ok(errno == 0xdeadbeef, "errno = %d\n", errno); + ok(ch == '2', "Unexpected char\n"); + + /* invalid file name */ + new = freopen("_:", "rb", file); + ok(new == NULL, "Shouldn't be able to reopen FILE\n"); + + /* don't close file, it's invalid by now */ + } + unlink(filename1); unlink(filename2); }
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/file.c | 4 +--- dlls/msvcrt/tests/file.c | 10 ++-------- 2 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c index e72784eef41..24d801a5306 100644 --- a/dlls/msvcrt/file.c +++ b/dlls/msvcrt/file.c @@ -4588,9 +4588,7 @@ FILE* CDECL _wfreopen(const wchar_t *path, const wchar_t *mode, FILE* file) TRACE(":path (%s) mode (%s) file (%p) fd (%d)\n", debugstr_w(path), debugstr_w(mode), file, file ? file->_file : -1);
LOCK_FILES(); - if (!file || ((fd = file->_file) < 0)) - file = NULL; - else + if (file) { fclose(file); if (msvcrt_get_flags(mode, &open_flags, &stream_flags) == -1) diff --git a/dlls/msvcrt/tests/file.c b/dlls/msvcrt/tests/file.c index 106c1554518..4c62a890459 100644 --- a/dlls/msvcrt/tests/file.c +++ b/dlls/msvcrt/tests/file.c @@ -1237,20 +1237,15 @@ static void test_freopen( void ) file->_file = -1;
new = freopen(filename2, "rb", file); - todo_wine ok(new == file, "freopen() didn't return same FILE*\n"); - if (new) { /* temporary as it crashes on wine */
- fd = fileno(new); - todo_wine + fd = fileno(file); ok(fd > 0, "fileno() returned %d\n", fd);
errno = 0xdeadbeef; ch = '#'; - ret = fread(&ch, 1, 1, new); - todo_wine + ret = fread(&ch, 1, 1, file); ok(ret == 1, "fread() returned %d\n", ret); - todo_wine ok(errno == 0xdeadbeef, "errno = %d\n", errno); ok(ch == '2', "Unexpected char\n");
@@ -1259,7 +1254,6 @@ static void test_freopen( void ) ok(new == NULL, "Shouldn't be able to reopen FILE\n");
/* don't close file, it's invalid by now */ - }
unlink(filename1); unlink(filename2);
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=136291
Your paranoid android.
=== debian11b (64 bit WoW report) ===
wldap32: parse: Timeout
On Wed Aug 16 15:26:48 2023 +0000, eric pouech wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/3578/diffs?diff_id=62906&start_sha=8c4b5cd4d4dba3ce77123bf6e4b6a489443b0d36#1a28d67ae45bfb1e18198f92ca618b484c4e5a63_2002_1947)
V2 pushed without requiring additional process - moved these tests without stdin as we no longer control its state - but sufficient to show the issue
Piotr Caban (@piotr) commented about dlls/msvcrt/tests/file.c:
- ok(fd > 0, "fileno() returned %d\n", fd);
- errno = 0xdeadbeef;
- ch = '#';
- ret = fread(&ch, 1, 1, new);
- todo_wine
- ok(ret == 1, "fread() returned %d\n", ret);
- todo_wine
- ok(errno == 0xdeadbeef, "errno = %d\n", errno);
- ok(ch == '2', "Unexpected char\n");
- /* invalid file name */
- new = freopen("_:", "rb", file);
- ok(new == NULL, "Shouldn't be able to reopen FILE\n");
- /* don't close file, it's invalid by now */
The commit message needs updating. Since the patch adds some questionable code that is removed in next patch maybe it's better to merge patches 2 and 3.
I think it's better to call fclose and check that it fails instead of adding the comment: ```suggestion:-0+0 ok(fclose(file) == EOF, "fclose(file) succeeded\n"); ```
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin -
Piotr Caban (＠piotr)