[PATCH 1/2] comctl32/tests: test toolbar separator with uninitialized iString
Trying to add a separator with an invalid pointer in iString to a toolbar works in Windows, showing that the field is ignored. --- dlls/comctl32/tests/toolbar.c | 51 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 51 insertions(+), 0 deletions(-) diff --git a/dlls/comctl32/tests/toolbar.c b/dlls/comctl32/tests/toolbar.c index 1946095..91a72e0 100644 --- a/dlls/comctl32/tests/toolbar.c +++ b/dlls/comctl32/tests/toolbar.c @@ -1300,6 +1300,56 @@ static void test_getstring(void) DestroyWindow(hToolbar); } +static void test_deadbeef(void) +{ + HWND hToolbar = NULL; + TBBUTTON buttons[2]; + INT r; + + rebuild_toolbar_with_buttons(&hToolbar); + + ZeroMemory(&buttons, sizeof(buttons)); + + /* Some programs (e.g. Graphmatica 2.0f) forget to initialize the iString + * field for separators. If the random bit pattern of this field looks like + * a pointer, trying to follow it casues a crash (pagefault). This does not + * happen in Windows XP, 2008 or Vista, suggesting that this field is + * ignored in Windows in this case. */ + buttons[0].idCommand = 0; + buttons[0].fsStyle = BTNS_SEP; + buttons[0].fsState = TBSTATE_ENABLED; + buttons[0].iString = 0xdeadbeef; + +#if 0 + /* It is also possible that a poorly written program doesn't initialize the + * iString field of an actual button. Different versions of Windows seem to + * operate differently in this case. + * + Windows XP doesn't seem to be bothered in the least + * + Windows 2008 crashes + * + Windows Vista does not crash, but the subsequent TB_ADDBUTTONS + * call fails + * Since I know of no program that suffers from this particular issue, I'm + * commenting out this part of the test for the time being. If it gets + * uncommented, the subsequent TB_ADDBUTTONS should be adjusted + * appropriately. */ + + buttons[1].idCommand = 1; + buttons[1].fsStyle = BTNS_BUTTON; + buttons[1].fsState = 0; + buttons[1].iString = 0xdeadbeef; +#endif + + r = SendMessage(hToolbar, TB_ADDBUTTONS, 1, (LPARAM)buttons); + expect(1, r); + + /* TODO: another test that might be worth doing, to get more insight on how + * Windows handles things, would be to introduce a separator with + * 0xdeadbeef as iString, and then change its style to make into a real + * button. */ + + DestroyWindow(hToolbar); +} + START_TEST(toolbar) { WNDCLASSA wc; @@ -1336,6 +1386,7 @@ START_TEST(toolbar) test_dispinfo(); test_setrows(); test_getstring(); + test_deadbeef(); PostQuitMessage(0); while(GetMessageA(&msg,0,0,0)) { -- 1.6.2.254.g84bde
Some programs (e.g. Graphmatica 2.0f) forget to initialize the iString field for separators in toolbars. Windows copes with this smoothly, ignoring the field altogether. Do the same in Wine. --- dlls/comctl32/toolbar.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/dlls/comctl32/toolbar.c b/dlls/comctl32/toolbar.c index c3add77..83098cc 100644 --- a/dlls/comctl32/toolbar.c +++ b/dlls/comctl32/toolbar.c @@ -1841,7 +1841,9 @@ TOOLBAR_InternalInsertButtonsT(TOOLBAR_INFO *infoPtr, INT iIndex, UINT nAddButto btnPtr->fsState = lpTbb[iButton].fsState; btnPtr->fsStyle = lpTbb[iButton].fsStyle; btnPtr->dwData = lpTbb[iButton].dwData; - if(HIWORD(lpTbb[iButton].iString) && lpTbb[iButton].iString != -1) + if(btnPtr->fsStyle & BTNS_SEP) + btnPtr->iString = -1; + else if(HIWORD(lpTbb[iButton].iString) && lpTbb[iButton].iString != -1) { if (fUnicode) Str_SetPtrW((LPWSTR*)&btnPtr->iString, (LPWSTR)lpTbb[iButton].iString ); -- 1.6.2.254.g84bde
Giuseppe Bilotta wrote:
Trying to add a separator with an invalid pointer in iString to a toolbar works in Windows, showing that the field is ignored. --- dlls/comctl32/tests/toolbar.c | 51 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 51 insertions(+), 0 deletions(-)
Hi Giuseppe, Patches should only go to wine-patches unless you'd like them to be reviewed first. You are sending them to both wine-patches and wine-devel. -- Cheers, Paul.
On Thu, Apr 23, 2009 at 8:00 AM, Paul Vriens <paul.vriens.wine(a)gmail.com> wrote:
Giuseppe Bilotta wrote:
Trying to add a separator with an invalid pointer in iString to a toolbar works in Windows, showing that the field is ignored.
Patches should only go to wine-patches unless you'd like them to be reviewed first.
You are sending them to both wine-patches and wine-devel.
Oh, sorry. Should I then send them to wine-devel for review first (possibly more then once) and after they get accepted send them to wine-patches? -- Giuseppe "Oblomov" Bilotta
Giuseppe Bilotta wrote:
On Thu, Apr 23, 2009 at 8:00 AM, Paul Vriens <paul.vriens.wine(a)gmail.com> wrote:
Giuseppe Bilotta wrote:
Trying to add a separator with an invalid pointer in iString to a toolbar works in Windows, showing that the field is ignored. Patches should only go to wine-patches unless you'd like them to be reviewed first.
You are sending them to both wine-patches and wine-devel.
Oh, sorry. Should I then send them to wine-devel for review first (possibly more then once) and after they get accepted send them to wine-patches?
There is no need for a specific review on wine-devel if you are comfortable with the patches. Patches sent to wine-patches will be reviewed as well. -- Cheers, Paul.
Hi, I think it would be interesting to test what TB_GETBUTTONINFO returns as iString for such a separator. That way we will know if the value is simply ignored, or it's something more complicated.
+static void test_deadbeef(void)
I think you should find a better name. Maybe test_addbuttons would be better - some more tests for this message could be added later?
+#if 0 + /* It is also possible that a poorly written program doesn't initialize the + * iString field of an actual button. Different versions of Windows seem to + * operate differently in this case. + * + Windows XP doesn't seem to be bothered in the least + * + Windows 2008 crashes + * + Windows Vista does not crash, but the subsequent TB_ADDBUTTONS + * call fails + * Since I know of no program that suffers from this particular issue, I'm + * commenting out this part of the test for the time being. If it gets + * uncommented, the subsequent TB_ADDBUTTONS should be adjusted + * appropriately. */
Tests with #if 0 are discouraged - usually after some time and changes in the surrounding, the code doesn't compile anymore. Probably leaving only the comment would be appropriate in this case. It's interesting that the behaviour changed with the version of Windows. You were testing with a program without a manifest? (i.e. using comctl32 v5.82. But it's hard to make a mistake, as if you used a manifest, you would have a lot of failures in check_sizes) Mikołaj
2009/4/23 Mikołaj Zalewski <mikolaj(a)zalewski.pl>:
Hi, I think it would be interesting to test what TB_GETBUTTONINFO returns as iString for such a separator. That way we will know if the value is simply ignored, or it's something more complicated.
You're definitely on to something here. Indeed, trying to get the TEXT button info causes a crash in Windows XP and Vista. Of course, during normal usage nobody is going to have a look at the iString of a separator.
+static void test_deadbeef(void)
I think you should find a better name. Maybe test_addbuttons would be better - some more tests for this message could be added later?
As per your suggestion, I named it test_invalidstring
Tests with #if 0 are discouraged - usually after some time and changes in the surrounding, the code doesn't compile anymore. Probably leaving only the comment would be appropriate in this case.
On #wine-devel it was suggested that #if 0 is the appropriate choice for tests that (can) crash on Windows. I think that having the code around, ready for testing, would be appropriate, at the very least until we decide what the Wine behaviour in these cases should be.
It's interesting that the behaviour changed with the version of Windows. You were testing with a program without a manifest? (i.e. using comctl32 v5.82. But it's hard to make a mistake, as if you used a manifest, you would have a lot of failures in check_sizes)
I'm testing using comctl32_crosstest as built by make crosstest, using mingw32 shipped by debian for x86-64. I assume that one doesn't have a manifest. It would seem that Windows XP (1) doesn't bother checking the pointers at all and (2) doesn't access the pointer itself until the button label is needed. Windows 2008 and Windows Vista instead access the pointer at button insertion time, but not when the button is a separator. Additionally, Windows 2008 does not check the pointer. Instead, Windows Vista checks the pointer and makes the insertion fail when it's invalid. I think that Vista's (apparent) approach is the most sensible: abort insertion without crashing on invalid pointer, unless we're handling a separator. Accept a separator with invalid address, keeping the address as-is. This might lead to a crash subsequently, if the text is accessed, but that happens in Windows too so it's fine. -- Giuseppe "Oblomov" Bilotta
Giuseppe Bilotta wrote:
2009/4/23 Mikołaj Zalewski <mikolaj(a)zalewski.pl>:
Hi, I think it would be interesting to test what TB_GETBUTTONINFO returns as iString for such a separator. That way we will know if the value is simply ignored, or it's something more complicated.
You're definitely on to something here. Indeed, trying to get the TEXT button info causes a crash in Windows XP and Vista. Of course, during normal usage nobody is going to have a look at the iString of a separator.
+static void test_deadbeef(void)
I think you should find a better name. Maybe test_addbuttons would be better - some more tests for this message could be added later?
As per your suggestion, I named it test_invalidstring
Tests with #if 0 are discouraged - usually after some time and changes in the surrounding, the code doesn't compile anymore. Probably leaving only the comment would be appropriate in this case.
On #wine-devel it was suggested that #if 0 is the appropriate choice for tests that (can) crash on Windows. I think that having the code around, ready for testing, would be appropriate, at the very least until we decide what the Wine behaviour in these cases should be.
if (0) would be better in that case as code will still be compiled. -- Cheers, Paul.
2009/4/23 Mikołaj Zalewski <mikolaj(a)zalewski.pl>:
Hi, I think it would be interesting to test what TB_GETBUTTONINFO returns as iString for such a separator. That way we will know if the value is simply ignored, or it's something more complicated.
Forget what I said in my previous email. I was running the GETBUTTONINFO the wrong way. I'm now using r = SendMessage(hToolbar, TB_GETBUTTONTEXT, 0, (LPARAM)NULL); to get the button text length (not caring about the button text itself). I found a number of things. (1) WINE always returns -1 in this case, which is wrong. I'll provide a patch for this (2) All versions of windows totally ignore the iString field for separators: valid or invalid, the resulting button text is always empty (-1 length). The behaviour of an invalid iString pointer for buttons seems to be undefined instead, crashes on some versions on button insertion, in some other on GETBUTTONTEXT, in other cases not at all. -- Giuseppe "Oblomov" Bilotta
participants (3)
-
Giuseppe Bilotta -
Mikołaj Zalewski -
Paul Vriens