Dmitry Timoshkov dmitry@baikal.ru writes:
- for (i = 0; i < sizeof(bstr)/sizeof(bstr[0]); i++)
- {
INTERNAL_BSTR *data = Get(bstr[i]);for (j = 0; j < i; j++) str[j] = '0' + i % 10;if (data->dwLen == i * sizeof(WCHAR))good_length_entries++;if (!memcmp(data->szString, str, i * sizeof(WCHAR)))good_string_entries++;- }
- trace("good_length_entries %d, good_string_entries %d\n", good_length_entries, good_string_entries);
- ok(good_length_entries >= 95, "good_length_entries %d out of 256\n", good_length_entries);
+todo_wine
- ok(good_string_entries >= 190, "good_string_entries %d out of 256\n", good_string_entries);
That doesn't seem very useful. What use case would there be for an app to rely on some random 75% of its strings to remain valid?
Alexandre Julliard julliard@winehq.org wrote:
That doesn't seem very useful. What use case would there be for an app to rely on some random 75% of its strings to remain valid?
Although the test is about statistics, my intention was to show that BSTR cache in Wine corrupts cached strings. I could try to figure out exact steps which lead to corruption of real strings the application cares about, but that's pretty hard to do, there are enourmous amount of lines in the log that are related to BSTR creation/destruction, and finally the string the app cares about and tries to use is corrupted. I thought that adding a statistical test would hint Jacek what might be wrong with current cache implementation.
Hi Dmitry,
On 06/05/12 13:05, Dmitry Timoshkov wrote:
Alexandre Julliard julliard@winehq.org wrote:
That doesn't seem very useful. What use case would there be for an app to rely on some random 75% of its strings to remain valid?
Although the test is about statistics, my intention was to show that BSTR cache in Wine corrupts cached strings. I could try to figure out exact steps which lead to corruption of real strings the application cares about, but that's pretty hard to do, there are enourmous amount of lines in the log that are related to BSTR creation/destruction, and finally the string the app cares about and tries to use is corrupted. I thought that adding a statistical test would hint Jacek what might be wrong with current cache implementation.
Unfortunately, statistical test isn't much helpful. The corruption you can see is probably due to the fact that not all strings are cached. Once the cache is full (for strings of similar size), the string is immediately freed. We can't cache all of them, that would be a massive leak. When I wrote the patch, I've seen the exact app using BSTRs in a pattern that I've verified that should work with the cache (or better said, the bug is hidden by the cache). I understand that it may be not easy in some cases, but I don't see how we could fix the implementation without understanding the cause of the problem.
BTW, did you test the app on Windows with disabled BSTR cache? It's easy to do by setting OANOCACHE environment variable before running the app. If that breaks the app, then indeed the app is broken in a way that it depends on BSTR cache behaviour.
Cheers, Jacek
Jacek Caban jacek@codeweavers.com wrote:
BTW, did you test the app on Windows with disabled BSTR cache? It's easy to do by setting OANOCACHE environment variable before running the app. If that breaks the app, then indeed the app is broken in a way that it depends on BSTR cache behaviour.
It's easy to test under Wine too. Yes, the app depends on enabled BSTR cache.
On 06/05/12 13:31, Dmitry Timoshkov wrote:
Jacek Caban jacek@codeweavers.com wrote:
BTW, did you test the app on Windows with disabled BSTR cache? It's easy to do by setting OANOCACHE environment variable before running the app. If that breaks the app, then indeed the app is broken in a way that it depends on BSTR cache behaviour.
It's easy to test under Wine too. Yes, the app depends on enabled BSTR cache.
Then you don't eliminate possibility of use-after-free bugs in Wine code base.
Jacek
Jacek Caban jacek@codeweavers.com wrote:
BTW, did you test the app on Windows with disabled BSTR cache? It's easy to do by setting OANOCACHE environment variable before running the app. If that breaks the app, then indeed the app is broken in a way that it depends on BSTR cache behaviour.
It's easy to test under Wine too. Yes, the app depends on enabled BSTR cache.
Then you don't eliminate possibility of use-after-free bugs in Wine code base.
Of course. But if that wasn't clear enough I'll rephrase it: yes, I tested the affected application under Windows, and setting OANOCACHE breaks it. With native oleaut32.dll the app works perfectly under Wine, setting OANOCACHE breaks it same way as under Windows.
Dmitry Timoshkov dmitry@baikal.ru writes:
Alexandre Julliard julliard@winehq.org wrote:
That doesn't seem very useful. What use case would there be for an app to rely on some random 75% of its strings to remain valid?
Although the test is about statistics, my intention was to show that BSTR cache in Wine corrupts cached strings. I could try to figure out exact steps which lead to corruption of real strings the application cares about, but that's pretty hard to do, there are enourmous amount of lines in the log that are related to BSTR creation/destruction, and finally the string the app cares about and tries to use is corrupted. I thought that adding a statistical test would hint Jacek what might be wrong with current cache implementation.
Any cache is going to reuse space sooner or later, that doesn't mean there's something wrong with it. If an app relies on a specific string not being reused, we need a test for that exact allocation pattern so that we can make sure our cache works the same way for that case.
-
Alexandre Julliard -
Dmitry Timoshkov -
Jacek Caban