[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@winehq.org Reporter: alexboly@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
Fabian Maurer dark.shadow4@web.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|blocker |normal CC| |dark.shadow4@web.de
--- Comment #1 from Fabian Maurer dark.shadow4@web.de --- Not a blocker. native crypt32 fixes the issue?
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #2 from Alexandru Bolboaca alexboly@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #3 from Fabian Maurer dark.shadow4@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?
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #4 from Alexandru Bolboaca alexboly@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
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #5 from Fabian Maurer dark.shadow4@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #6 from Alexandru Bolboaca alexboly@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #7 from Fabian Maurer dark.shadow4@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?
https://bugs.winehq.org/show_bug.cgi?id=44405
Linards linards.liepins@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |linards.liepins@gmail.com
--- Comment #8 from Linards linards.liepins@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
Richard Yao ryao@gentoo.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ryao@gentoo.org
--- Comment #9 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #10 from Richard Yao ryao@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 ];
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #11 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #12 from Richard Yao ryao@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
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #13 from Nikolay Sivov bunglehead@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #14 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #15 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #16 from Nikolay Sivov bunglehead@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #17 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #18 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #19 from Nikolay Sivov bunglehead@gmail.com --- Return value doesn't matter, it's not used anywhere. I suggest to fix the export, with corresponding test.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #20 from Richard Yao ryao@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
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #21 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
Richard Yao ryao@gentoo.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #62204|0 |1 is obsolete| |
--- Comment #22 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
Richard Yao ryao@gentoo.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #62210|0 |1 is obsolete| |
--- Comment #23 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #24 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #25 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12@gmail.com
--- Comment #26 from Zebediah Figura z.figura12@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/
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #27 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #28 from Nikolay Sivov bunglehead@gmail.com --- I sent a patch for this one, https://www.winehq.org/pipermail/wine-devel/2018-September/132724.html.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #29 from Richard Yao ryao@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.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #30 from Nikolay Sivov bunglehead@gmail.com --- This should work now, d18d38bc98006ef7c3c34e8e4e744e3768835735. Please retest with current Wine, or with 3.17 when it's released.
https://bugs.winehq.org/show_bug.cgi?id=44405
--- Comment #31 from Nikolay Sivov bunglehead@gmail.com --- Please retest with Wine 3.17.
https://bugs.winehq.org/show_bug.cgi?id=44405
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |focht@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/
https://bugs.winehq.org/show_bug.cgi?id=44405
Józef Kucia joseph.kucia@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fjfrackiewicz@gmail.com
--- Comment #32 from Józef Kucia joseph.kucia@gmail.com --- *** Bug 40849 has been marked as a duplicate of this bug. ***
https://bugs.winehq.org/show_bug.cgi?id=44405
Józef Kucia joseph.kucia@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|UNCONFIRMED |RESOLVED CC| |joseph.kucia@gmail.com Fixed by SHA1| |d18d38bc98006ef7c3c34e8e4e7 | |44e3768835735
--- Comment #33 from Józef Kucia joseph.kucia@gmail.com --- Assuming fixed.
https://bugs.winehq.org/show_bug.cgi?id=44405
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #34 from Alexandre Julliard julliard@winehq.org --- Closing bugs fixed in 3.18.
-
wine-bugs＠winehq.org