[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: [e-mail]
        ReportedBy: [e-mail]
                CC: [e-mail]
    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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #1 from Jacek Caban  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?

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #2 from Dmitry Timoshkov  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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #3 from Dmitry Timoshkov  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?

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #4 from Jacek Caban  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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #5 from Dmitry Timoshkov  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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #6 from Dmitry Timoshkov  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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #7 from Dmitry Timoshkov  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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #8 from Dmitry Timoshkov  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?

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #9 from Jacek Caban  2012-06-21 06:47:39 CDT ---
Created attachment 40640
  --> http://bugs.winehq.org/attachment.cgi?id=40640
hack

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #10 from Jacek Caban  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).

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #11 from Dmitry Timoshkov  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?

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #12 from Jacek Caban  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

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #13 from Dmitry Timoshkov  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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #14 from Jacek Caban  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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #15 from Dmitry Timoshkov  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.

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

--- Comment #16 from Dmitry Timoshkov  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!

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |ed9d78d2b96dd4c97a6402ff0d3
                   |                            |0ada47a352a6f
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #17 from Dmitry Timoshkov  2013-02-14 01:27:29 CST ---
ed9d78d2b96dd4c97a6402ff0d30ada47a352a6f makes BSTR cache behave much better.
Thanks Jacek!

[Bug 30839] BSTR cache corrupts most of cached BSTR entries

http://bugs.winehq.org/show_bug.cgi?id=30839

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #18 from Alexandre Julliard  2013-02-15 14:30:53 CST ---
Closing bugs fixed in 1.5.24.