[Bug 30839] New: BSTR cache corrupts most of cached BSTR entries
http://bugs.winehq.org/show_bug.cgi?id=30839 Bug #: 30839 Summary: BSTR cache corrupts most of cached BSTR entries Product: Wine Version: 1.5.5 Platform: x86-64 OS/Version: Linux Status: NEW Keywords: testcase Severity: normal Priority: P2 Component: oleaut32 AssignedTo: wine-bugs(a)winehq.org ReportedBy: dmitry(a)baikal.ru CC: jacek(a)codeweavers.com Classification: Unclassified http://www.winehq.org/pipermail/wine-patches/2012-June/114723.html imitates what pretty large and complex application does, and shows that BSTR cache in Wine corrupts most of cached BSTR entries: vartype.c:6145: good_length_entries 225, good_string_entries 162 vartype.c:6148: Test failed: good_string_entries 162 out of 256 vartype.c:6164: good_length_entries 97, good_string_entries 67 vartype.c:6166: Test failed: good_length_entries 97 out of 256 vartype.c:6168: Test failed: good_string_entries 67 out of 256 Under Windows most of cached strings stay valid for pretty long time after SysFreeString() is called. Jacek, I'm adding you to the cc: list because you are the authour of BSTR cache in Wine. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #1 from Jacek Caban <jacek(a)codeweavers.com> 2012-06-05 10:02:49 CDT --- As we discussed on wine-devel, this kind of tests aren't something we may consider a valid bug in the code. Do you want to keep it open and replace the test with fixing BSTR cache for the pattern used by your application or should we resolve it as INVALID? -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #2 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-05 10:12:36 CDT --- (In reply to comment #1)
As we discussed on wine-devel, this kind of tests aren't something we may consider a valid bug in the code. Do you want to keep it open and replace the test with fixing BSTR cache for the pattern used by your application or should we resolve it as INVALID?
I'll try to figure out what is going on and create a new test. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #3 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-13 10:35:48 CDT --- (In reply to comment #1)
As we discussed on wine-devel, this kind of tests aren't something we may consider a valid bug in the code. Do you want to keep it open and replace the test with fixing BSTR cache for the pattern used by your application or should we resolve it as INVALID?
An investigation shows that commenting out HeapFree() in SysFreeString() fixes all the problems with corrupted strings in that large and complex application I have here. So, it appears that the problem is not that BSTR cache corrupts its entries, but that strings get corrupted because they don't get cached. Before closing this bug as invalid I'd like to ask: Jacek, do you have an idea how BSTR cache could be improved so that older strings get replaced by newer ones once the cache is full? -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #4 from Jacek Caban <jacek(a)codeweavers.com> 2012-06-13 10:51:07 CDT --- (In reply to comment #3)
An investigation shows that commenting out HeapFree() in SysFreeString() fixes all the problems with corrupted strings in that large and complex application I have here. So, it appears that the problem is not that BSTR cache corrupts its entries, but that strings get corrupted because they don't get cached.
Yeah, that's how the cache works. Any cache would work this way, implementations may differ only by when which string is freed.
Before closing this bug as invalid I'd like to ask: Jacek, do you have an idea how BSTR cache could be improved so that older strings get replaced by newer ones once the cache is full?
That's not exactly what happened when I was testing how BSTR cache is supposed to work. Again, we can't get exactly the same behavior as native unless you'd reverse engineer oleaut32. What we can do is make sure that some patterns work with our implementation, but we need to know that pattern first. To give you an example: str1 = SysAllocStringLen(..., 20); str2 = SysAllocStringLen(..., 20); SysFreeString(str1); SysFreeString(str2); DoSomethingNotAllocatingNewBSTRsWithStrings(str1, str2); is something guaranteed to work. Obviously it's a trivial example, but if your app really works reasonably well on Windows, it must follow another pattern that happens to work. Without understanding it, I don't see how we can fix the problem. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #5 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-13 11:07:29 CDT --- (In reply to comment #4)
str1 = SysAllocStringLen(..., 20); str2 = SysAllocStringLen(..., 20); SysFreeString(str1); SysFreeString(str2);
DoSomethingNotAllocatingNewBSTRsWithStrings(str1, str2);
is something guaranteed to work. Obviously it's a trivial example, but if your app really works reasonably well on Windows, it must follow another pattern that happens to work. Without understanding it, I don't see how we can fix the problem.
I'll give it another try and see if I can gather more details on what is going on. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #6 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-14 05:35:22 CDT --- Created attachment 40540 --> http://bugs.winehq.org/attachment.cgi?id=40540 test bstr cache Attached test case consistently passes under all versions of Windows before Vista (https://testbot.winehq.org/JobDetails.pl?Key=19048), but fails under Vista+ and Wine. Jacek, does this test case make sense to you? Unfortunately I can't test my application under Vista since it uses 32-bit driver for copy protection and my Vista is 64 bit. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #7 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-14 07:19:05 CDT --- (In reply to comment #6)
Unfortunately I can't test my application under Vista since it uses 32-bit driver for copy protection and my Vista is 64 bit.
The application doesn't work with 32-bit oleaut32.dll copied from Vista, while it does work with 32-bit oleaut32.dll copied from XP. Although it looks like the application is broken by newer oleaut32 bstr cache implementations, it works just fine with older versions. So, it should be feasible to make bstr cache in Wine compatible with those older versions. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #8 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-21 00:02:19 CDT --- Jacek, could you please have a look at the test and perhaps suggest what could be improved/changed in BSTR cache to make it pass? -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #9 from Jacek Caban <jacek(a)codeweavers.com> 2012-06-21 06:47:39 CDT --- Created attachment 40640 --> http://bugs.winehq.org/attachment.cgi?id=40640 hack -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #10 from Jacek Caban <jacek(a)codeweavers.com> 2012-06-21 06:58:07 CDT --- (In reply to comment #7)
The application doesn't work with 32-bit oleaut32.dll copied from Vista, while it does work with 32-bit oleaut32.dll copied from XP.
Although it looks like the application is broken by newer oleaut32 bstr cache implementations,
If the application depends on bstr cache in any way it's obviously broken.
Jacek, could you please have a look at the test and perhaps suggest what could be improved/changed in BSTR cache to make it pass?
I've attached a patch that may help. Although it's technically good, I don't it in Wine yet. Did you try running your test case on Windows with FLG_HEAP_ENABLE_FREE_CHECK set? What you observe could be just another heap management (that is, the cache frees the memory, but it's not yet corrupted by heap management). -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #11 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-21 07:11:02 CDT --- (In reply to comment #10)
If the application depends on bstr cache in any way it's obviously broken.
That's true. Unfortunately Wine needs to support such broken applications.
I've attached a patch that may help. Although it's technically good, I don't it in Wine yet.
I'll give it a go tomorrow.
Did you try running your test case on Windows with FLG_HEAP_ENABLE_FREE_CHECK set? What you observe could be just another heap management (that is, the cache frees the memory, but it's not yet corrupted by heap management).
I'd certainly try it out. How this kind of tests could be enabled? -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #12 from Jacek Caban <jacek(a)codeweavers.com> 2012-06-21 07:14:52 CDT --- (In reply to comment #11)
I'd certainly try it out. How this kind of tests could be enabled?
The easiest way seems to be setting it by registry. See: http://msdn.microsoft.com/en-us/library/windows/hardware/ff542901%28v=vs.85%... and dlls/kernel32/heap.c -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #13 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-21 22:04:25 CDT --- (In reply to comment #10)
I've attached a patch that may help.
This patch makes the test consistently pass, and with that patch the application I worry about works as well.
Although it's technically good, I don't it in Wine yet.
What is the reason for that? What are the drawbacks if that patch is committed?
Did you try running your test case on Windows with FLG_HEAP_ENABLE_FREE_CHECK set? What you observe could be just another heap management (that is, the cache frees the memory, but it's not yet corrupted by heap management).
Enabling FLG_HEAP_ENABLE_FREE_CHECK with gflags.exe changes nothing. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #14 from Jacek Caban <jacek(a)codeweavers.com> 2012-06-22 07:57:13 CDT --- (In reply to comment #13)
(In reply to comment #10)
I've attached a patch that may help.
This patch makes the test consistently pass, and with that patch the application I worry about works as well.
Great.
Although it's technically good, I don't it in Wine yet.
What is the reason for that? What are the drawbacks if that patch is committed?
This patch keeps a buffer in a bucked for smaller buffers if accurate bucked is full. This was we keep it alive for longer time, but at cost of wasted memory. We already do similar trick when we look for a buffer in cache (if we don't have matching buffer, we try to use a bit bigger one). If we keep extending the cache implementation this way, we risk a serious waste of memory, which I'd much rather avoid in such a common memory allocator. So, with this patch, the semantic is preserved, but it needs more thoughts. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #15 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-06-22 08:29:52 CDT --- (In reply to comment #14)
This patch keeps a buffer in a bucked for smaller buffers if accurate bucked is full. This was we keep it alive for longer time, but at cost of wasted memory. We already do similar trick when we look for a buffer in cache (if we don't have matching buffer, we try to use a bit bigger one). If we keep extending the cache implementation this way, we risk a serious waste of memory, which I'd much rather avoid in such a common memory allocator. So, with this patch, the semantic is preserved, but it needs more thoughts.
I played a bit with the affected application + the patch, and looked for memory consumption: I didn't notice any visible change in memory usage statistics in comparison with one without the patch. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 --- Comment #16 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2012-09-25 00:44:50 CDT --- Hi Jacek, I'd like to avoid adding your patch to private branches again and again, and it has proved to fix the problems with corrupted BSTR entries in some large applications around here during pretty long testing period, please consider sending this patch for inclusion. Thanks! -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 Dmitry Timoshkov <dmitry(a)baikal.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |ed9d78d2b96dd4c97a6402ff0d3 | |0ada47a352a6f Status|NEW |RESOLVED Resolution| |FIXED --- Comment #17 from Dmitry Timoshkov <dmitry(a)baikal.ru> 2013-02-14 01:27:29 CST --- ed9d78d2b96dd4c97a6402ff0d30ada47a352a6f makes BSTR cache behave much better. Thanks Jacek! -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=30839 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #18 from Alexandre Julliard <julliard(a)winehq.org> 2013-02-15 14:30:53 CST --- Closing bugs fixed in 1.5.24. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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