[Bug 44310] New: WritePrivateProfileString doesn't check for flush errors
https://bugs.winehq.org/show_bug.cgi?id=44310 Bug ID: 44310 Summary: WritePrivateProfileString doesn't check for flush errors Product: Wine Version: 3.0-rc5 Hardware: x86 OS: Linux Status: UNCONFIRMED Severity: minor Priority: P2 Component: kernel32 Assignee: wine-bugs(a)winehq.org Reporter: development(a)winomega.com Distribution: --- Created attachment 60148 --> https://bugs.winehq.org/attachment.cgi?id=60148 test case The Windows version of WritePrivateProfileString reports an error if flushing to file failed. See attached test case. It tries to write to Z:\test.ini (which is expected to fail as Z is mapped to /, with no write perms). Under wine I get no output from the test, and the file isn't created. Failure warnings do show up in +profile channel log: trace:profile:PROFILE_Open path: L"Z:\\test.ini" warn:profile:PROFILE_Open profile file L"Z:\\test.ini" not found trace:profile:PROFILE_SetString (L"Section1",L"FirstKey",L"It all worked out OK."): trace:profile:PROFILE_SetString creating key warn:profile:PROFILE_FlushFile could not save profile file L"Z:\\test.ini" (error was 5) warn:profile:PROFILE_FlushFile could not save profile file L"Z:\\test.ini" (error was 5) Under Windows XP (where Z: drive doesn't exist), I get "failed with error 3" message. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #1 from Omega Software <development(a)winomega.com> --- Created attachment 60149 --> https://bugs.winehq.org/attachment.cgi?id=60149 fix for current git Attached patch fixes the problem in current git, by checking for PROFILE_FlushFile result (instead of just PROFILE_SetString). -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Omega Software <development(a)winomega.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |patch, testcase -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #2 from Omega Software <development(a)winomega.com> --- For the records, this bug also affects Delphi applications that rely on TIniFile: WriteString should be rising an exception when it is unable to write, but it doesn't. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Fabian Maurer <dark.shadow4(a)web.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW CC| |dark.shadow4(a)web.de Ever confirmed|0 |1 --- Comment #3 from Fabian Maurer <dark.shadow4(a)web.de> --- Good catch, this is indeed problematic. You probably want to add a test case (should be easy enough) and then sent it to the mailing list once code-freeze is over. While you're at it, there are more places where PROFILE_FlushFile is called. Do these show the same erroneous behavior? Like in "WritePrivateProfileSectionW", "WritePrivateProfileStructW" Site not, I don't like chaining two methods that have side effects with "&&" like that. I'd probably prefer ------ ret = PROFILE_SetString( section, entry, string, FALSE); ret &= PROFILE_FlushFile(); ------ or ------ ret = PROFILE_SetString( section, entry, string, FALSE); if(ret) ret = PROFILE_FlushFile(); ------ But I don't know about the coding style, just wanted to note. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #4 from Omega Software <development(a)winomega.com> --- Hi! (In reply to Fabian Maurer from comment #3)
Good catch, this is indeed problematic. You probably want to add a test case (should be easy enough) and then sent it to the mailing list once code-freeze is over.
I would, but I'm not sure what directory can be used in the test suite that is guaranteed not to be writable. Any idea?
While you're at it, there are more places where PROFILE_FlushFile is called. Do these show the same erroneous behavior? Like in "WritePrivateProfileSectionW", "WritePrivateProfileStructW"
They do. I've extended both the test case and the patch (attaching).
Site not, I don't like chaining two methods that have side effects with "&&" like that. I'd probably prefer ------ ret = PROFILE_SetString( section, entry, string, FALSE); ret &= PROFILE_FlushFile();
Yes, much more readable this way. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Omega Software <development(a)winomega.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #60149|0 |1 is obsolete| | --- Comment #5 from Omega Software <development(a)winomega.com> --- Created attachment 60198 --> https://bugs.winehq.org/attachment.cgi?id=60198 fix for current git -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Omega Software <development(a)winomega.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #60148|0 |1 is obsolete| | --- Comment #6 from Omega Software <development(a)winomega.com> --- Created attachment 60199 --> https://bugs.winehq.org/attachment.cgi?id=60199 test case -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #7 from Fabian Maurer <dark.shadow4(a)web.de> --- Created attachment 60203 --> https://bugs.winehq.org/attachment.cgi?id=60203 test cases That's a good question actually. You most likely will have to create the needed setting yourself, i.e. make a location where it can't save the profile to. Setting the readonly attribute on the file is not enough, since wine already checks this. But you can use ACL to forbid everyone to write to the folder! I found that to be a though one, I attached two tests that you can use, if you want. It should already work fine, but I kinda created them in a hurry, so no warranty. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Omega Software <development(a)winomega.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #60198|0 |1 is obsolete| | Attachment #60199|0 |1 is obsolete| | --- Comment #8 from Omega Software <development(a)winomega.com> --- Created attachment 60216 --> https://bugs.winehq.org/attachment.cgi?id=60216 fix + testcase Thanks! That was really helpful. I've adapted your directory test function to use the same dummy test values and generate similar output as the other tests in the same file. Here's the new patch including test case. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #9 from Fabian Maurer <dark.shadow4(a)web.de> ---
ok(ret == FALSE, "Expected FALSE, got %d\n", ret);
I don't think you need it like that, since were dealing with a boolean here. When it's not FALSE, it must be TRUE. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #10 from Omega Software <development(a)winomega.com> --- That's how it's done in the rest of dlls/kernel32/tests/profile.c. See: ret = WritePrivateProfileStringA(NULL, "key", "string", path); ok(ret == FALSE, "Expected FALSE, got %d\n", ret); ok(GetLastError() == ERROR_FILE_NOT_FOUND || -- ret = WritePrivateProfileStringA(NULL, "key", "string", path); ok(ret == FALSE, "Expected FALSE, got %d\n", ret); ok(GetLastError() == ERROR_FILE_NOT_FOUND || -- ret = WritePrivateProfileStringA("App", "key", "string", ""); ok(ret == FALSE, "Expected FALSE, got %d\n", ret); ok(GetLastError() == ERROR_ACCESS_DENIED || -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #11 from Fabian Maurer <dark.shadow4(a)web.de> --- Oh I see... Well, I'd still say it's superfluous. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #12 from Alexandre Julliard <julliard(a)winehq.org> --- (In reply to Fabian Maurer from comment #11)
Oh I see... Well, I'd still say it's superfluous.
It probably doesn't matter in this case, but note that there's nothing that says that a boolean must be TRUE, any non-zero value would do. So printing it is not necessarily useless. For the same reason, your &= trick is not a good idea. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #13 from Fabian Maurer <dark.shadow4(a)web.de> ---
It probably doesn't matter in this case, but note that there's nothing that says that a boolean must be TRUE, any non-zero value would do. So printing it is not necessarily useless.
For the same reason, your &= trick is not a good idea.
Is that "&=" a real concern at this point? Because I think it makes it easier to read while it's pretty easy to check that it's safe. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #14 from Alexandre Julliard <julliard(a)winehq.org> --- (In reply to Fabian Maurer from comment #13)
It probably doesn't matter in this case, but note that there's nothing that says that a boolean must be TRUE, any non-zero value would do. So printing it is not necessarily useless.
For the same reason, your &= trick is not a good idea.
Is that "&=" a real concern at this point? Because I think it makes it easier to read while it's pretty easy to check that it's safe.
It may look easier on the surface, but using an incorrect construct is actually more confusing for people reading the code. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #15 from Fabian Maurer <dark.shadow4(a)web.de> ---
It may look easier on the surface, but using an incorrect construct is actually more confusing for people reading the code.
I see, thanks for explaining. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #16 from Omega Software <development(a)winomega.com> --- Which way to go, then? Should we use this: if(ret) ret = PROFILE_FlushFile(); ? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #17 from Alexandre Julliard <julliard(a)winehq.org> --- (In reply to Omega Software from comment #16)
Which way to go, then? Should we use this:
if(ret) ret = PROFILE_FlushFile();
?
Yes this one is fine. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Omega Software <development(a)winomega.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #60216|0 |1 is obsolete| | --- Comment #18 from Omega Software <development(a)winomega.com> --- Created attachment 60231 --> https://bugs.winehq.org/attachment.cgi?id=60231 fix + testcase Ok here is it. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #19 from Omega Software <development(a)winomega.com> --- Hi I sent the patch to https://www.winehq.org/pipermail/wine-devel/2018-January/121762.html I assume it is waiting for approval now. Please tell me if you need anything else. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #20 from Omega Software <development(a)winomega.com> --- Based on instructions from https://wiki.winehq.org/Developer_FAQ#I_sent_a_patch.2C_but_it_got_ignored._... I'm going to submit only the conformance test first, as my first attempt didn't catch Julliard's attention. Fabian, may I have your permission to put your name in Signed-off-by for this part, along with mine? (the code is mostly yours after all) Thanks -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #21 from Fabian Maurer <dark.shadow4(a)web.de> --- I'm going to submit only the conformance test first, as my first
attempt didn't catch Julliard's attention.
Well, it might take some more time, but splitting it into two parts can't harm, I guess. But I'd send both patches at once, as a patchset. Just make sure to mark the tests as todo_wine in the first patch, and remove the todo_wine with the patch that fixes the issue.
Fabian, may I have your permission to put your name in Signed-off-by for this part, along with mine? (the code is mostly yours after all)
Sure thing, but please CC me when sending it. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Fabian Maurer <dark.shadow4(a)web.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Fixed by SHA1| |39899fd11a8ff9228a587adf5e9 | |aeaf2ded93c5c Status|NEW |RESOLVED --- Comment #22 from Fabian Maurer <dark.shadow4(a)web.de> --- Congrats, you got it in already - no need for resending. marking fixed by 39899fd11a8ff9228a587adf5e9aeaf2ded93c5c. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 --- Comment #23 from Omega Software <development(a)winomega.com> --- Excellent! Thanks -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #24 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 3.1. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Michael Stefaniuc <mstefani(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |3.0.x -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=44310 Michael Stefaniuc <mstefani(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|3.0.x |--- --- Comment #25 from Michael Stefaniuc <mstefani(a)winehq.org> --- Removing the 3.0.x milestone from bugs included in 3.0.1. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
wine-bugs＠winehq.org