[Bug 44405] New: Rise Of Nations Extended edition, Steam version crashes with page fault when trying to list mods
https://bugs.winehq.org/show_bug.cgi?id=44405 Bug ID: 44405 Summary: Rise Of Nations Extended edition, Steam version crashes with page fault when trying to list mods Product: Wine Version: 3.0 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: blocker Priority: P2 Component: crypt32 Assignee: wine-bugs(a)winehq.org Reporter: alexboly(a)gmail.com Distribution: --- Run steam version of Rise of Nations extended edition (32 bits) on Ubuntu 16.04 LTS (64 bits) with Nvidia GeForce GTX 660M, proprietary driver 384.111. When it tries to list mods from Steam Workshop, a page fault happens. Excerpt from log: page fault on write access to 0x00000000 in 32-bit code Backtrace: =>0 0x7e3bb761 in crypt32 (+0xb761) (0x0033eef8) 1 0x7e3bce4a CryptBinaryToStringW+0x229() in crypt32 (0x0033ef5c) Complete log attached. -- 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=44405 Fabian Maurer <dark.shadow4(a)web.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|blocker |normal CC| |dark.shadow4(a)web.de --- Comment #1 from Fabian Maurer <dark.shadow4(a)web.de> --- Not a blocker. native crypt32 fixes the issue? -- 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=44405 --- Comment #2 from Alexandru Bolboaca <alexboly(a)gmail.com> --- (In reply to Fabian Maurer from comment #1)
Not a blocker. native crypt32 fixes the issue?
Thanks. I tried native crypt32, didn't work. -- 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=44405 --- Comment #3 from Fabian Maurer <dark.shadow4(a)web.de> --- I guess there is no way to test that without having to buy the game, right? Can you provide a +crypt log? Also, do you know how to apply a patch and compile from source? -- 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=44405 --- Comment #4 from Alexandru Bolboaca <alexboly(a)gmail.com> --- (In reply to Fabian Maurer from comment #3)
I guess there is no way to test that without having to buy the game, right?
I'm afraid I can't think of one, no.
Can you provide a +crypt log? Also, do you know how to apply a patch and compile from source?
I can provide a crypt log, I'll just have to find it. And yes, I can apply a patch, recompile etc, if you point me to the project instructions. I'm a programmer, just never worked on wine until now, and my C/C++ is a little rusty :). Would debugging help as well? (Again, I could use some instructions for setting up the environment). Thank you and have a nice week-end, Alex -- 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=44405 --- Comment #5 from Fabian Maurer <dark.shadow4(a)web.de> --- Created attachment 60350 --> https://bugs.winehq.org/attachment.cgi?id=60350 Dump CryptBinaryToStringW parameter You can create a log by running wine from the terminal, like "WINEDEBUG=+crypt wine WHATEVER.EXE &> wine.log" For on how to compile wine, see the FAQ: https://wiki.winehq.org/Building_Wine A 32bit built is the easiest, and enough. Usually just cloning the git repo, running "./configure" and then "make" is enough though. I attached a patch for dumping the binary that it tries to convert and fails. If you could apply that patch, recompile and then create a log of the program with "WINEDEBUG=+crypt /whatever/path/wine-git/wine PROGRAM &> wine.log", I should be able to reproduce the issue. -- 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=44405 --- Comment #6 from Alexandru Bolboaca <alexboly(a)gmail.com> --- Thanks for your support. Quick update: I'm working on it and I hope to have an update by the end of next week. (In reply to Fabian Maurer from comment #5)
Created attachment 60350 [details] Dump CryptBinaryToStringW parameter
You can create a log by running wine from the terminal, like "WINEDEBUG=+crypt wine WHATEVER.EXE &> wine.log"
For on how to compile wine, see the FAQ: https://wiki.winehq.org/Building_Wine A 32bit built is the easiest, and enough. Usually just cloning the git repo, running "./configure" and then "make" is enough though.
I attached a patch for dumping the binary that it tries to convert and fails. If you could apply that patch, recompile and then create a log of the program with "WINEDEBUG=+crypt /whatever/path/wine-git/wine PROGRAM &> wine.log", I should be able to reproduce the issue.
-- 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=44405 --- Comment #7 from Fabian Maurer <dark.shadow4(a)web.de> ---
Quick update: I'm working on it and I hope to have an update by the end of next week.
By working on it, do you mean you're working on a fix? -- 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=44405 Linards <linards.liepins(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |linards.liepins(a)gmail.com --- Comment #8 from Linards <linards.liepins(a)gmail.com> --- Can confirm that issue is somewhere in the integration with Steam. When running riseofnations.exe from the directory via terminal, all runs perfectly 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=44405 Richard Yao <ryao(a)gentoo.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ryao(a)gentoo.org --- Comment #9 from Richard Yao <ryao(a)gentoo.org> --- The issue is in dlls/crypt32/base64.c:328 according to the backtrace proton gave me: https://github.com/ValveSoftware/Proton/issues/298#issuecomment-417895690 Replacing crypt32 with wine tricks and setting an override via winecfg works around the problem, so I am inclined to think that that wine's crypt32 isn't doing the right thing here. -- 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=44405 --- Comment #10 from Richard Yao <ryao(a)gentoo.org> --- To make it more clear, the problem is that encodeBase64W is trying to dereference invalid memory. It looks like it is being done on this line: /* first char is the first 6 bits of the first byte*/ *ptr++ = b64[ ( d[0] >> 2) & 0x3f ]; -- 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=44405 --- Comment #11 from Richard Yao <ryao(a)gentoo.org> --- Created attachment 62204 --> https://bugs.winehq.org/attachment.cgi?id=62204 This fixes the segfault. I took a peek at this and I understand what is wrong. This commit was incorrect when it was done 9 years ago: https://source.winehq.org/git/wine.git/commitdiff/2d5ac92d9a6878785158301b90... BinaryToBase64W() will invoke `encodeBase64W(pbBinary, cbBinary, sep, NULL, &charsNeeded);` The NULL becomes `out_buf` in `encodeBase64W()`. It then invokes `ptr = out_buf;`, followed by: *ptr++ = b64[ ( d[0] >> 2) & 0x3f ]; That is our NULL pointer dereference. The only way this code could have worked would be if the caller did something wrong, causing it to exit early with ERROR_INSUFFICIENT_BUFFER. When invoked with a NULL, the correct thing to do appears to be to return early because it looks like the code just wants a calculation to be done of how much space is actually needed. I have written and tested a small patch designed to do this and it makes the game work. -- 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=44405 --- Comment #12 from Richard Yao <ryao(a)gentoo.org> --- I want to submit this patch to wine-devel@, but I seem to need to be subscribed to the mailing list before I can do that. I am waiting for the mailing list administrator to add me to the list. I plan to submit it after I am added to the list, but I submitted this to Proton to hopefully get it merged there sooner: https://github.com/ValveSoftware/wine/pull/15 -- 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=44405 --- Comment #13 from Nikolay Sivov <bunglehead(a)gmail.com> --- (In reply to Richard Yao from comment #12)
I want to submit this patch to wine-devel@, but I seem to need to be subscribed to the mailing list before I can do that. I am waiting for the mailing list administrator to add me to the list. I plan to submit it after I am added to the list, but I submitted this to Proton to hopefully get it merged there sooner:
This doesn't look right. NULL output is used internally to query for needed buffer length, so it should hit this return: --- 307 if (needed > *out_len) 308 { 309 *out_len = needed; 310 return ERROR_INSUFFICIENT_BUFFER; 311 } --- The issue is probably in CryptBinaryToStringW export itself, that does not check for NULL output buffer. This needs tests. -- 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=44405 --- Comment #14 from Richard Yao <ryao(a)gentoo.org> --- (In reply to Nikolay Sivov from comment #13)
(In reply to Richard Yao from comment #12)
I want to submit this patch to wine-devel@, but I seem to need to be subscribed to the mailing list before I can do that. I am waiting for the mailing list administrator to add me to the list. I plan to submit it after I am added to the list, but I submitted this to Proton to hopefully get it merged there sooner:
This doesn't look right. NULL output is used internally to query for needed buffer length, so it should hit this return:
--- 307 if (needed > *out_len) 308 { 309 *out_len = needed; 310 return ERROR_INSUFFICIENT_BUFFER; 311 } ---
The issue is probably in CryptBinaryToStringW export itself, that does not check for NULL output buffer. This needs tests.
It won't hit that when needed is 0, which can happen for certain inputs. -- 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=44405 --- Comment #15 from Richard Yao <ryao(a)gentoo.org> --- (In reply to Nikolay Sivov from comment #13)
The issue is probably in CryptBinaryToStringW export itself, that does not check for NULL output buffer. This needs tests.
This is incorrect. CryptBinaryToStringW is not being called. BinaryToBase64W is. However, BinaryToBase64W is marked static, so the unwinding code mistakenly identifies the function in the call frame as CryptBinaryToStringW. What is happening is that BinaryToBase64W is calling encodeBase64W to do a length calculation with a NULL pointer. It should return, but it instead tries to write out to the NULL pointer, causing the segmentation fault. -- 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=44405 --- Comment #16 from Nikolay Sivov <bunglehead(a)gmail.com> --- BinaryToBase64W is only used from CryptBinaryToStringW, it's not exported. And 'needed' can't be 0. But anyway, let's wait for the patch to come through. -- 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=44405 --- Comment #17 from Richard Yao <ryao(a)gentoo.org> --- (In reply to Nikolay Sivov from comment #16)
BinaryToBase64W is only used from CryptBinaryToStringW, it's not exported.
I made a mistake when interpreting the backtraces. Our call stack is really: encodeBase64W BinaryToBase64W CryptBinaryToStringW We are segfaulting in encodeBase64W, which itself is static and is only called by BinaryToBase64W. That is called by CryptBinaryToStringW.
And 'needed' can't be 0. But anyway, let's wait for the patch to come through.
It must be 0 here because 1. The code in BinaryToBase64W is: charsNeeded = 0; encodeBase64W(pbBinary, cbBinary, sep, NULL, &charsNeeded); 2. The branch is not being taken because it is segfaulting further down in the function according to the backtrace from Proton: https://github.com/ValveSoftware/Proton/issues/298 3. The backtrace shows out_buf is NULL and the precise line identified is the first attempt to dereference it. This feels like bug #44583. After thinking about this some more, I think there are two issues: 1. The calculation of the value of `needed` is likely incorrect given that the function is trying to write out data when the calculation claims that we need zero data. Someone needs to figure out what the correct calculation is. I agree that wine needs test cases here. 2. Fixing the calculation of `needed` would cause us to return ERROR_INSUFFICIENT_BUFFER, which does not seem right to me. Calling this function with a NULL buffer is to rely on the side effect of *out_buf being updated and my intuition is that such a thing should always return success. I think both solutions are necessary for correctness, although either one would likely be sufficient to make Rise of Nations stop crashing. -- 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=44405 --- Comment #18 from Richard Yao <ryao(a)gentoo.org> --- Would it be acceptable to say that this bug is for encodeBase64W not returning success when given a NULL out_buf while bug #44583 is for encodeBase64W doing an incorrect calculation? If not, then we have a redundant bug. -- 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=44405 --- Comment #19 from Nikolay Sivov <bunglehead(a)gmail.com> --- Return value doesn't matter, it's not used anywhere. I suggest to fix the export, with corresponding test. -- 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=44405 --- Comment #20 from Richard Yao <ryao(a)gentoo.org> --- (In reply to Nikolay Sivov from comment #19)
Return value doesn't matter, it's not used anywhere. I suggest to fix the export, with corresponding test.
I see what you are saying. I don't know if I have time for that right now, but I agree that is the correct fix. My patch was only meant to make it stop crashing (which seems to work in this case) under the assumption that everything else was sane. That assumption is clearly wrong. Assuming that I find time to do this, would I need to know anything about writing conformance tests beyond what is written on the wiki? https://wiki.winehq.org/Wine_Developer%27s_Guide/Writing_Conformance_Tests -- 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=44405 --- Comment #21 from Richard Yao <ryao(a)gentoo.org> --- For future reference, someone already wrote a test case: https://bugs.winehq.org/show_bug.cgi?id=44583 If the test case is good, then we just need to fix the function to match the test. -- 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=44405 Richard Yao <ryao(a)gentoo.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62204|0 |1 is obsolete| | --- Comment #22 from Richard Yao <ryao(a)gentoo.org> --- Created attachment 62210 --> https://bugs.winehq.org/attachment.cgi?id=62210 A slightly better fix I think I at least partially figured it out. Microsoft's documentation for CryptBinaryToStringW() states that if pszString is NULL, then the function calculates the length and returns it via pcchString: https://docs.microsoft.com/en-us/windows/desktop/api/wincrypt/nf-wincrypt-cr... Presumably, if pszString is NULL, then we should never read *pcchString because it is allowed to be undefined, but the code does this anyway. This is wrong. It is still not 100% clear how we are not bailing (partly because attaching winedbg with /tmp/proton_winedbg_run fails), but it is clear to me that we are supposed to unconditionally bail early when given a NULL pointer, which we are not doing. As for test cases, the Microsoft documentation is unclear enough on other aspects of this function that I have nothing that I could use as a reference for what a correct test case should be. The right way here could be to pick some inputs, run them against the original DLL and then hard code the outputs as the test cases. However, this might require another person do that in order for it to count as clean room engineering rather than reverse engineering. I live in the United States where reverse engineering is legally problematic. The best that I could do here is rely on the code comments, Microsoft's incomplete documentation and whether this stops things from crashing to design test cases, which doesn't seem right to me. -- 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=44405 Richard Yao <ryao(a)gentoo.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62210|0 |1 is obsolete| | --- Comment #23 from Richard Yao <ryao(a)gentoo.org> --- Created attachment 62211 --> https://bugs.winehq.org/attachment.cgi?id=62211 Take 3 The previous patch had a small mistake in it. This addresses that. -- 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=44405 --- Comment #24 from Richard Yao <ryao(a)gentoo.org> --- One last remark. Hot keys are not working in Rise of Nations when using the wine crypt32. I am not sure if it is because I did something wrong when fixing this or if it is another bug. -- 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=44405 --- Comment #25 from Richard Yao <ryao(a)gentoo.org> --- After some more thought, I think the solution is install Windows in a VM and develop the conformance tests there. Sorry for the noise. I am new to this. -- 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=44405 Zebediah Figura <z.figura12(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12(a)gmail.com --- Comment #26 from Zebediah Figura <z.figura12(a)gmail.com> --- (In reply to Richard Yao from comment #25)
After some more thought, I think the solution is install Windows in a VM and develop the conformance tests there. Sorry for the noise. I am new to this.
In fact we have test machines available for usage, so you don't necessarily have to install your own: https://testbot.winehq.org/ -- 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=44405 --- Comment #27 from Richard Yao <ryao(a)gentoo.org> --- (In reply to Zebediah Figura from comment #26)
(In reply to Richard Yao from comment #25)
After some more thought, I think the solution is install Windows in a VM and develop the conformance tests there. Sorry for the noise. I am new to this.
In fact we have test machines available for usage, so you don't necessarily have to install your own: https://testbot.winehq.org/
I registered for an account. I’ll likely find time this weekend to use it if it is approved. Thanks for telling me about that. -- 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=44405 --- Comment #28 from Nikolay Sivov <bunglehead(a)gmail.com> --- I sent a patch for this one, https://www.winehq.org/pipermail/wine-devel/2018-September/132724.html. -- 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=44405 --- Comment #29 from Richard Yao <ryao(a)gentoo.org> --- (In reply to Nikolay Sivov from comment #28)
I sent a patch for this one, https://www.winehq.org/pipermail/wine-devel/2018-September/132724.html.
Awesome. I will try it out on my next wine rebuild. Thanks for taking the time to do this. As I said on GitHub, I unfortunately was unable to make time to finish pursuing this. -- 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=44405 --- Comment #30 from Nikolay Sivov <bunglehead(a)gmail.com> --- This should work now, d18d38bc98006ef7c3c34e8e4e744e3768835735. Please retest with current Wine, or with 3.17 when it's released. -- 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=44405 --- Comment #31 from Nikolay Sivov <bunglehead(a)gmail.com> --- Please retest with Wine 3.17. -- 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=44405 Anastasius Focht <focht(a)gmx.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |focht(a)gmx.net Summary|Rise Of Nations Extended |Rise Of Nations Extended |edition, Steam version |edition (Steam) crashes |crashes with page fault |when trying to list mods |when trying to list mods |due to missing NULL output | |buffer handling in | |CryptBinaryToString() URL| |https://store.steampowered. | |com/app/287450/ -- 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=44405 Józef Kucia <joseph.kucia(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fjfrackiewicz(a)gmail.com --- Comment #32 from Józef Kucia <joseph.kucia(a)gmail.com> --- *** Bug 40849 has been marked as a duplicate of this bug. *** -- 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=44405 Józef Kucia <joseph.kucia(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|UNCONFIRMED |RESOLVED CC| |joseph.kucia(a)gmail.com Fixed by SHA1| |d18d38bc98006ef7c3c34e8e4e7 | |44e3768835735 --- Comment #33 from Józef Kucia <joseph.kucia(a)gmail.com> --- Assuming fixed. -- 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=44405 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #34 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 3.18. -- 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