[PATCH] msvcrt: Fix fscanf return when EOF is immediately after an end of line.
Hi Erich, The implementation part of the patch doesn't look right. I think that it should be fixed by %s format reporting error when there's no characters to read. I'm attaching a diff (generated on top of your patch) that shows the problems. Also please use only tests on strings (sscanf) or handle file creation failure. Thanks, Piotr
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=57904 Your paranoid android. === build (build log) === error: patch failed: dlls/msvcrt/tests/scanf.c:27 Task: Patch failed to apply === debian10 (build log) === error: patch failed: dlls/msvcrt/tests/scanf.c:27 Task: Patch failed to apply === debian10 (build log) === error: patch failed: dlls/msvcrt/tests/scanf.c:27 Task: Patch failed to apply
Thanks Piotr! I'm happy to change that, I was just using the fprintf tests as a template. Does the attached look better to you? Best, Erich On Thu, Oct 17, 2019 at 5:23 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
The implementation part of the patch doesn't look right. I think that it should be fixed by %s format reporting error when there's no characters to read.
I'm attaching a diff (generated on top of your patch) that shows the problems.
Also please use only tests on strings (sscanf) or handle file creation failure.
Thanks, Piotr
Hi Erich, On 10/17/19 5:41 PM, Erich E. Hoover wrote:
+ /* if we have reached the EOF and output nothing then report EOF */ + if (nch==_EOF_ && rd==0 && st==0) { _UNLOCK_FILE_(file); + return _EOF_RET; + }
Please also update the widecharstring case.
+ fp = fopen(file_name, "wb"); + ok(fp, "fp = %p\n", fp); This cause compilation warning.
Thanks, Piotr
Hi Piotr, I just ran the widecharstring case on the testbot ( https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to return 0 instead of WEOF... Are you alright with just adding this test to demonstrate that it doesn't work the same way? Failing test: === buffer[0] = ' '; buffer[1] = '\t'; buffer[2] = '\n'; buffer[3] = '\n'; buffer[4] = 0; ret = swscanf(buffer, formats, results); ok( ret == (short)WEOF, "ret = %d\n", ret ); === Best, Erich On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
On 10/17/19 5:41 PM, Erich E. Hoover wrote:
+ /* if we have reached the EOF and output nothing then report EOF */ + if (nch==_EOF_ && rd==0 && st==0) { _UNLOCK_FILE_(file); + return _EOF_RET; + }
Please also update the widecharstring case.
+ fp = fopen(file_name, "wb"); + ok(fp, "fp = %p\n", fp); This cause compilation warning.
Thanks, Piotr
Hi Erich, I've done some testing and here's what I found: - swscanf in msvcrt returns 0 as you have described - swscanf in msvcr90 returns WEOF - __stdio_common_vswscanf in ucrtbase returns WEOF It looks like there's a bug/backward compatibility code in native msvcrt.dll. It would be good to do some more testing on different *wscanf functions in msvcrt. I guess that the way to go is to: - add swscanf tests in msvcrt (you can use L"" there) - add swscanf tests in one of the newer dlls - make the implementation change specific to newer versions of C-runtime Hope it helps, Piotr On 10/18/19 5:40 PM, Erich E. Hoover wrote:
Hi Piotr,
I just ran the widecharstring case on the testbot ( https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to return 0 instead of WEOF... Are you alright with just adding this test to demonstrate that it doesn't work the same way? Failing test: === buffer[0] = ' '; buffer[1] = '\t'; buffer[2] = '\n'; buffer[3] = '\n'; buffer[4] = 0; ret = swscanf(buffer, formats, results); ok( ret == (short)WEOF, "ret = %d\n", ret ); ===
Best, Erich
On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
On 10/17/19 5:41 PM, Erich E. Hoover wrote:
+ /* if we have reached the EOF and output nothing then report EOF */ + if (nch==_EOF_ && rd==0 && st==0) { _UNLOCK_FILE_(file); + return _EOF_RET; + }
Please also update the widecharstring case.
+ fp = fopen(file_name, "wb"); + ok(fp, "fp = %p\n", fp); This cause compilation warning.
Thanks, Piotr
Wow, thanks Piotr! Are you sure you were testing against msvcr90? I just ran a couple tries against the testbot and I see it doing what we expect on msvcr120, but msvcr90 it's giving 0 ( https://testbot.winehq.org/JobDetails.pl?Key=57997 ). I'll go through and establish exactly what version they fixed this in, but I want to make sure that there isn't something weird going on with msvcr90. Best, Erich On Fri, Oct 18, 2019 at 12:45 PM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
I've done some testing and here's what I found: - swscanf in msvcrt returns 0 as you have described - swscanf in msvcr90 returns WEOF - __stdio_common_vswscanf in ucrtbase returns WEOF
It looks like there's a bug/backward compatibility code in native msvcrt.dll. It would be good to do some more testing on different *wscanf functions in msvcrt.
I guess that the way to go is to: - add swscanf tests in msvcrt (you can use L"" there) - add swscanf tests in one of the newer dlls - make the implementation change specific to newer versions of C-runtime
Hope it helps, Piotr
On 10/18/19 5:40 PM, Erich E. Hoover wrote:
Hi Piotr,
I just ran the widecharstring case on the testbot ( https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to return 0 instead of WEOF... Are you alright with just adding this test to demonstrate that it doesn't work the same way? Failing test: === buffer[0] = ' '; buffer[1] = '\t'; buffer[2] = '\n'; buffer[3] = '\n'; buffer[4] = 0; ret = swscanf(buffer, formats, results); ok( ret == (short)WEOF, "ret = %d\n", ret ); ===
Best, Erich
On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
On 10/17/19 5:41 PM, Erich E. Hoover wrote:
+ /* if we have reached the EOF and output nothing then report EOF */ + if (nch==_EOF_ && rd==0 && st==0) { _UNLOCK_FILE_(file); + return _EOF_RET; + }
Please also update the widecharstring case.
+ fp = fopen(file_name, "wb"); + ok(fp, "fp = %p\n", fp); This cause compilation warning.
Thanks, Piotr
Hi, The test is not failing on msvcr120 only because it’s skipped on all machines. The tests are linked agains msvcrt.dll, you need to load the function dynamically. Thanks, Piotr
On 18 Oct 2019, at 21:15, Erich E. Hoover <erich.e.hoover(a)gmail.com> wrote:
Wow, thanks Piotr!
Are you sure you were testing against msvcr90? I just ran a couple tries against the testbot and I see it doing what we expect on msvcr120, but msvcr90 it's giving 0 ( https://testbot.winehq.org/JobDetails.pl?Key=57997 ). I'll go through and establish exactly what version they fixed this in, but I want to make sure that there isn't something weird going on with msvcr90.
Best, Erich
On Fri, Oct 18, 2019 at 12:45 PM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
I've done some testing and here's what I found: - swscanf in msvcrt returns 0 as you have described - swscanf in msvcr90 returns WEOF - __stdio_common_vswscanf in ucrtbase returns WEOF
It looks like there's a bug/backward compatibility code in native msvcrt.dll. It would be good to do some more testing on different *wscanf functions in msvcrt.
I guess that the way to go is to: - add swscanf tests in msvcrt (you can use L"" there) - add swscanf tests in one of the newer dlls - make the implementation change specific to newer versions of C-runtime
Hope it helps, Piotr
On 10/18/19 5:40 PM, Erich E. Hoover wrote:
Hi Piotr,
I just ran the widecharstring case on the testbot ( https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to return 0 instead of WEOF... Are you alright with just adding this test to demonstrate that it doesn't work the same way? Failing test: === buffer[0] = ' '; buffer[1] = '\t'; buffer[2] = '\n'; buffer[3] = '\n'; buffer[4] = 0; ret = swscanf(buffer, formats, results); ok( ret == (short)WEOF, "ret = %d\n", ret ); ===
Best, Erich
On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
On 10/17/19 5:41 PM, Erich E. Hoover wrote:
+ /* if we have reached the EOF and output nothing then report EOF */ + if (nch==_EOF_ && rd==0 && st==0) { _UNLOCK_FILE_(file); + return _EOF_RET; + }
Please also update the widecharstring case.
+ fp = fopen(file_name, "wb"); + ok(fp, "fp = %p\n", fp); This cause compilation warning.
Thanks, Piotr
Ah, well - glad I asked :) How much specificity do you want in figuring out where this got fixed? (since there are no tests before msvcr90) Best, Erich On Fri, Oct 18, 2019 at 1:22 PM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi,
The test is not failing on msvcr120 only because it’s skipped on all machines.
The tests are linked agains msvcrt.dll, you need to load the function dynamically.
Thanks, Piotr
On 18 Oct 2019, at 21:15, Erich E. Hoover <erich.e.hoover(a)gmail.com> wrote:
Wow, thanks Piotr!
Are you sure you were testing against msvcr90? I just ran a couple tries against the testbot and I see it doing what we expect on msvcr120, but msvcr90 it's giving 0 ( https://testbot.winehq.org/JobDetails.pl?Key=57997 ). I'll go through and establish exactly what version they fixed this in, but I want to make sure that there isn't something weird going on with msvcr90.
Best, Erich
On Fri, Oct 18, 2019 at 12:45 PM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
I've done some testing and here's what I found: - swscanf in msvcrt returns 0 as you have described - swscanf in msvcr90 returns WEOF - __stdio_common_vswscanf in ucrtbase returns WEOF
It looks like there's a bug/backward compatibility code in native msvcrt.dll. It would be good to do some more testing on different *wscanf functions in msvcrt.
I guess that the way to go is to: - add swscanf tests in msvcrt (you can use L"" there) - add swscanf tests in one of the newer dlls - make the implementation change specific to newer versions of C-runtime
Hope it helps, Piotr
On 10/18/19 5:40 PM, Erich E. Hoover wrote:
Hi Piotr,
I just ran the widecharstring case on the testbot ( https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to return 0 instead of WEOF... Are you alright with just adding this test to demonstrate that it doesn't work the same way? Failing test: === buffer[0] = ' '; buffer[1] = '\t'; buffer[2] = '\n'; buffer[3] = '\n'; buffer[4] = 0; ret = swscanf(buffer, formats, results); ok( ret == (short)WEOF, "ret = %d\n", ret ); ===
Best, Erich
On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
On 10/17/19 5:41 PM, Erich E. Hoover wrote:
+ /* if we have reached the EOF and output nothing then report EOF */ + if (nch==_EOF_ && rd==0 && st==0) { _UNLOCK_FILE_(file); + return _EOF_RET; + }
Please also update the widecharstring case.
+ fp = fopen(file_name, "wb"); + ok(fp, "fp = %p\n", fp); This cause compilation warning.
Thanks, Piotr
Hi Erich, msvcr80 is the first dll that was changed. Attaching a hacky test that shows it (it also shows difference is %S format handling but it's not something that has to be fixed as part of your patch). Thanks, Piotr On 10/18/19 9:30 PM, Erich E. Hoover wrote:
Ah, well - glad I asked :) How much specificity do you want in figuring out where this got fixed? (since there are no tests before msvcr90)
Best, Erich
On Fri, Oct 18, 2019 at 1:22 PM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi,
The test is not failing on msvcr120 only because it’s skipped on all machines.
The tests are linked agains msvcrt.dll, you need to load the function dynamically.
Thanks, Piotr
On 18 Oct 2019, at 21:15, Erich E. Hoover <erich.e.hoover(a)gmail.com> wrote:
Wow, thanks Piotr!
Are you sure you were testing against msvcr90? I just ran a couple tries against the testbot and I see it doing what we expect on msvcr120, but msvcr90 it's giving 0 ( https://testbot.winehq.org/JobDetails.pl?Key=57997 ). I'll go through and establish exactly what version they fixed this in, but I want to make sure that there isn't something weird going on with msvcr90.
Best, Erich
On Fri, Oct 18, 2019 at 12:45 PM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
I've done some testing and here's what I found: - swscanf in msvcrt returns 0 as you have described - swscanf in msvcr90 returns WEOF - __stdio_common_vswscanf in ucrtbase returns WEOF
It looks like there's a bug/backward compatibility code in native msvcrt.dll. It would be good to do some more testing on different *wscanf functions in msvcrt.
I guess that the way to go is to: - add swscanf tests in msvcrt (you can use L"" there) - add swscanf tests in one of the newer dlls - make the implementation change specific to newer versions of C-runtime
Hope it helps, Piotr
On 10/18/19 5:40 PM, Erich E. Hoover wrote:
Hi Piotr,
I just ran the widecharstring case on the testbot ( https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to return 0 instead of WEOF... Are you alright with just adding this test to demonstrate that it doesn't work the same way? Failing test: === buffer[0] = ' '; buffer[1] = '\t'; buffer[2] = '\n'; buffer[3] = '\n'; buffer[4] = 0; ret = swscanf(buffer, formats, results); ok( ret == (short)WEOF, "ret = %d\n", ret ); ===
Best, Erich
On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Erich,
On 10/17/19 5:41 PM, Erich E. Hoover wrote: > + /* if we have reached the EOF and output nothing then report EOF */ > + if (nch==_EOF_ && rd==0 && st==0) { _UNLOCK_FILE_(file); > + return _EOF_RET; > + }
Please also update the widecharstring case.
> + fp = fopen(file_name, "wb"); > + ok(fp, "fp = %p\n", fp); This cause compilation warning.
Thanks, Piotr
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=58117 Your paranoid android. === w2008s64 (32 bit report) === Report errors: The report seems to have been truncated === w2008s64 (64 bit report) === Report errors: The report seems to have been truncated
On Mon, 21 Oct 2019, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=58117
Your paranoid android.
=== w2008s64 (32 bit report) ===
Report errors: The report seems to have been truncated
=== w2008s64 (64 bit report) ===
Report errors: The report seems to have been truncated
The reason for these errors is that Windows fails to start the executable because it cannot find microsoft.vc80.crt. When starting the patched executable by hand I get the following error message (on stderr which is unfortunately not captured): The application has failed to start because its side-by-side configuration is incorrect. Please see the application event log for more detail. Looking in the event log I see: Activation context generation failed for "C:\Users\Administrator\Documents\msvcr90_test.exe". Dependent Assembly microsoft.vc80.crt,language="*",processorArchitecture="*",publicKeyToken="1fc8b3b9a1e18e3b",type="win32",version="8.0.50727.4053" could not be found. Please use sxstrace.exe for detailed diagnosis. -- Francois Gouget <fgouget(a)codeweavers.com>
On Mon, Oct 21, 2019 at 10:29 AM Francois Gouget <fgouget(a)codeweavers.com> wrote:
On Mon, 21 Oct 2019, Marvin wrote:
The reason for these errors is that Windows fails to start the executable because it cannot find microsoft.vc80.crt. ...
So, what you're saying is that the reason we don't have tests before msvcr90 is that not all the testbots can run them. That makes sense, thanks! Best, Erich
On 10/21/19 7:17 PM, Erich E. Hoover wrote:
On Mon, Oct 21, 2019 at 10:29 AM Francois Gouget <fgouget(a)codeweavers.com> wrote:
On Mon, 21 Oct 2019, Marvin wrote:
The reason for these errors is that Windows fails to start the executable because it cannot find microsoft.vc80.crt. ...
So, what you're saying is that the reason we don't have tests before msvcr90 is that not all the testbots can run them. That makes sense, thanks! It was only an explanation why the tests didn't run. If it's added properly the tests will be skipped (based on tested dll name).
There's no reason to not add tests for msvcr80. On the other hand it's probably only useful to document differences between versions. Thanks, Piotr
On Mon, 21 Oct 2019, Erich E. Hoover wrote:
On Mon, Oct 21, 2019 at 10:29 AM Francois Gouget <fgouget(a)codeweavers.com> wrote:
On Mon, 21 Oct 2019, Marvin wrote:
The reason for these errors is that Windows fails to start the executable because it cannot find microsoft.vc80.crt. ...
So, what you're saying is that the reason we don't have tests before msvcr90 is that not all the testbots can run them. That makes sense, thanks!
Yes, it's possible some VMs are missing some dlls. Question: Do we want some test configurations where these optional components are not installed? In this case the description for w2008s64 says: Upgraded with Internet Explorer 9, .Net Framework 3.5 SP1 & 4.5.1 and all updates up to 2013/04/15. Added Visual C++ 2012 & 2013 runtimes. Whereas the one for w2003std says: Installed Internet Explorer 8, SP2 and all updates up to 2019/07/23. Added .Net 2.0 SP1, 3.0 SP2, 3.5 SP2 & 4, msxml4 SP3, Visual C++ 2005 SP1, 2008 SP1 & 2010 SP1 runtimes. msvcr80 is part of Visual C++ 2005 so it should be present on w2003std. For w2008s64 I think I determined that the Visual C++ 2005 runtime was already installed and thus did not manually install it. In c:\windows\winsxs I do see the following files: 01/19/2008 05:11 AM <DIR> amd64_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.1434_none_88de292b2fb06019 04/08/2013 06:58 AM <DIR> amd64_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.3053_none_88e044e32fae7230 04/08/2013 07:50 AM <DIR> amd64_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.4016_none_88dc01492fb256de 04/08/2013 07:03 AM <DIR> amd64_microsoft.vc90.crt_1fc8b3b9a1e18e3b_9.0.21022.8_none_750b37ff97f4f68b 01/19/2008 05:11 AM <DIR> x86_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.1434_none_d08b6002442c891f 04/08/2013 06:58 AM <DIR> x86_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.3053_none_d08d7bba442a9b36 04/08/2013 07:50 AM <DIR> x86_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.4016_none_d0893820442e7fe4 04/08/2013 07:02 AM <DIR> x86_microsoft.vc90.crt_1fc8b3b9a1e18e3b_9.0.21022.8_none_bcb86ed6ac711f91 But they are all slightly older than the version required by the patch (8.0.50727.4053). -- Francois Gouget <fgouget(a)codeweavers.com>
On 10/21/19 8:04 PM, Francois Gouget wrote:
Question: Do we want some test configurations where these optional components are not installed? I don't think there's much value in having machines without these dlls. If it's not too much work it's probably worth adding to more machines.
Thanks, Piotr
On Mon, 21 Oct 2019, Francois Gouget wrote: [...]
In this case the description for w2008s64 says:
Upgraded with Internet Explorer 9, .Net Framework 3.5 SP1 & 4.5.1 and all updates up to 2013/04/15. Added Visual C++ 2012 & 2013 runtimes.
So running vcredist-2005sp1_x86.exe does not change anything. However running Windows Update a few times does install the expected msvcr80 dll in the end. So I have added w2008s64 to the list of VMs to update. -- Francois Gouget <fgouget(a)codeweavers.com>
On Mon, Oct 21, 2019 at 4:33 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
... msvcr80 is the first dll that was changed. Attaching a hacky test that shows it (it also shows difference is %S format handling but it's not something that has to be fixed as part of your patch).
Thanks Piotr, I was trying to do this on Friday and didn't realize that the manifest needed to be changed for it to work. At some point is it worth adding tests for these older msvcr* dlls so that it's easier to test against them? Anyway, I believe that this version now has all of the changes that you have requested - so please let me know if I missed something. Best, Erich
Hi Erich, The patch looks mostly good now but it causes compilation warnings. The prototype of swscanf function is incorrect: +static int (__cdecl *p_swscanf)(const char *str, const wchar_t* format, ...); Also result variable is not used. Thanks, Piotr On 10/21/19 6:00 PM, Erich E. Hoover wrote:
On Mon, Oct 21, 2019 at 4:33 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
... msvcr80 is the first dll that was changed. Attaching a hacky test that shows it (it also shows difference is %S format handling but it's not something that has to be fixed as part of your patch).
Thanks Piotr, I was trying to do this on Friday and didn't realize that the manifest needed to be changed for it to work. At some point is it worth adding tests for these older msvcr* dlls so that it's easier to test against them?
Anyway, I believe that this version now has all of the changes that you have requested - so please let me know if I missed something.
Best, Erich
On Mon, Oct 21, 2019 at 11:51 AM Piotr Caban <piotr.caban(a)gmail.com> wrote:
... The patch looks mostly good now but it causes compilation warnings. The prototype of swscanf function is incorrect: +static int (__cdecl *p_swscanf)(const char *str, const wchar_t* format, ...); Also result variable is not used.
Whoops, sorry about that. I thought I had "-Werror" turned on to catch such things and didn't realize that it now needs to be added to CROSSCFLAGS as well. Best, Erich
participants (5)
-
Erich E. Hoover -
Francois Gouget -
Marvin -
Piotr Caban -
Piotr Caban