[PATCH] comdlg32: Avoid crash in RemoveControlItem.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51640 Signed-off-by: Bernhard Übelacker <bernhardu(a)mailbox.org> --- dlls/comdlg32/itemdlg.c | 3 +++ dlls/comdlg32/tests/itemdlg.c | 37 +++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/dlls/comdlg32/itemdlg.c b/dlls/comdlg32/itemdlg.c index d6957d51a66..da1a289bc5a 100644 --- a/dlls/comdlg32/itemdlg.c +++ b/dlls/comdlg32/itemdlg.c @@ -4156,6 +4156,9 @@ static HRESULT WINAPI IFileDialogCustomize_fnRemoveControlItem(IFileDialogCustom item = get_item(ctrl, dwIDItem, CDCS_VISIBLE|CDCS_ENABLED, &position); + if (!item) + return E_INVALIDARG; + if ((item->cdcstate & (CDCS_VISIBLE|CDCS_ENABLED)) == (CDCS_VISIBLE|CDCS_ENABLED)) { if(SendMessageW(ctrl->hwnd, CB_DELETESTRING, position, 0) == CB_ERR) diff --git a/dlls/comdlg32/tests/itemdlg.c b/dlls/comdlg32/tests/itemdlg.c index c38457a0a13..e6060acdf5d 100644 --- a/dlls/comdlg32/tests/itemdlg.c +++ b/dlls/comdlg32/tests/itemdlg.c @@ -2460,6 +2460,42 @@ static void test_overwrite(void) IShellItem_Release(psi_current); } +static void test_customize_remove_from_empty_combobox(void) +{ + IFileDialog *pfod; + IFileDialogCustomize *pfdc; + UINT i; + HRESULT hr; + hr = CoCreateInstance(&CLSID_FileOpenDialog, NULL, CLSCTX_INPROC_SERVER, + &IID_IFileDialog, (void**)&pfod); + ok(hr == S_OK, "got 0x%08x.\n", hr); + + hr = IFileDialog_QueryInterface(pfod, &IID_IFileDialogCustomize, (void**)&pfdc); + ok(hr == S_OK, "got 0x%08x.\n", hr); + if(FAILED(hr)) + { + skip("Skipping IFileDialogCustomize tests.\n"); + IFileDialog_Release(pfod); + return; + } + + i = 107; + hr = IFileDialogCustomize_AddComboBox(pfdc, i); + ok(hr == S_OK, "got 0x%08x.\n", hr); + + hr = IFileDialogCustomize_RemoveAllControlItems(pfdc, i); + ok(hr == E_NOTIMPL, "got 0x%08x.\n", hr); + + hr = IFileDialogCustomize_SetSelectedControlItem(pfdc, i, 1000); + ok(hr == E_INVALIDARG, "got 0x%08x.\n", hr); + + hr = IFileDialogCustomize_RemoveControlItem(pfdc, i, 0); + ok(hr == E_INVALIDARG, "got 0x%08x.\n", hr); + + IFileDialogCustomize_Release(pfdc); + IFileDialog_Release(pfod); +} + START_TEST(itemdlg) { OleInitialize(NULL); @@ -2474,6 +2510,7 @@ START_TEST(itemdlg) test_customize(); test_persistent_state(); test_overwrite(); + test_customize_remove_from_empty_combobox(); } else skip("Skipping all Item Dialog tests.\n"); -- 2.33.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=103829 Your paranoid android. === w7u_2qxl (32 bit report) === comdlg32: itemdlg: Timeout === w7u_adm (32 bit report) === comdlg32: itemdlg: Timeout === w7u_el (32 bit report) === comdlg32: itemdlg: Timeout === w1064v1507 (32 bit report) === comdlg32: itemdlg: Timeout === w1064v1809 (32 bit report) === comdlg32: itemdlg: Timeout === w1064 (32 bit report) === comdlg32: itemdlg: Timeout === w1064_tsign (32 bit report) === comdlg32: itemdlg: Timeout === w10pro64 (32 bit report) === comdlg32: itemdlg: Timeout === w1064v1507 (64 bit report) === comdlg32: itemdlg: Timeout === w1064v1809 (64 bit report) === comdlg32: itemdlg: Timeout === w1064 (64 bit report) === comdlg32: itemdlg: Timeout === w1064_2qxl (64 bit report) === comdlg32: itemdlg: Timeout === w1064_tsign (64 bit report) === comdlg32: itemdlg: Timeout === w10pro64 (64 bit report) === comdlg32: itemdlg: Timeout === w10pro64_ar (64 bit report) === comdlg32: itemdlg: Timeout === w10pro64_he (64 bit report) === comdlg32: itemdlg: Timeout === w10pro64_ja (64 bit report) === comdlg32: itemdlg: Timeout === w10pro64_zh_CN (64 bit report) === comdlg32: itemdlg: Timeout
Am 10.12.21 um 13:09 schrieb Marvin:
=== w7u_2qxl (32 bit report) ===
comdlg32: itemdlg: Timeout
...
=== w1064v1507 (32 bit report) ===
comdlg32: itemdlg: Timeout
... Hello Francois, I tried to submit today this patch, unfortunately it timed out at each Windows 7 and 10 VMs. It times out also with a patch that should not change behaviour when manually submitting. E.g. https://testbot.winehq.org/GetFile.pl?JobKey=103828 On the other side the test page for comdlg32:itemdlg is flawless: https://test.winehq.org/data/tests/comdlg32:itemdlg.html The testbot web interface presents these final screenshots. But they show just an empty desktop. I assume in my case winetest detects the runtime of more than 120 seconds, terminates the child process and ends itself, then the screenshot is taken? Is there a way to get the screenshot before the timeout of winetest.exe is reached? E.g. a possibility to reduce some kind of "outer" timeout? Kind regards, Bernhard
On Fri, 10 Dec 2021, Bernhard Übelacker wrote:
Am 10.12.21 um 13:09 schrieb Marvin:
=== w7u_2qxl (32 bit report) ===
comdlg32: itemdlg: Timeout
...
=== w1064v1507 (32 bit report) ===
comdlg32: itemdlg: Timeout
...
Hello Francois, I tried to submit today this patch, unfortunately it timed out at each Windows 7 and 10 VMs.
It times out also with a patch that should not change behaviour when manually submitting. E.g. https://testbot.winehq.org/GetFile.pl?JobKey=103828
You should be able to diagnose that by checking: Timestamp traces (WINETEST_TIME) and/or Report successful tests (WINETEST_REPORT_SUCCESS)
I assume in my case winetest detects the runtime of more than 120 seconds, terminates the child process and ends itself, then the screenshot is taken?
Usually yes. Technically there's a bit of a race between the timeout in Winetest.exe and the process monitoring the task on the TestBot server. But normally Winetest.exe is supposed to time out first. It looks like the 'stack trace' of the test getting stuck is something like this: itemdlg.c:1530:9.359 Test succeeded // test_filename() itemdlg.c:102:9.406 Test succeeded // get_hwnd_from_ifiledialog() itemdlg.c:221:9.422 Test succeeded // IFileDialogEvents_fnOnFolderChange() https://testbot.winehq.org/JobDetails.pl?Key=103851 It may be necessary to add some more traces to pinpoint exactly where the test gets stuck.
Is there a way to get the screenshot before the timeout of winetest.exe is reached? E.g. a possibility to reduce some kind of "outer" timeout?
There's a plan to take screenshots at a to-be-determined regular interval. Some progress has been made on it but there's a dozen other things to improve on the TestBot and at some point it fell by the wayside :-( https://bugs.winehq.org/show_bug.cgi?id=44709 -- Francois Gouget <fgouget(a)codeweavers.com>
Am 10.12.21 um 16:42 schrieb Francois Gouget:
On Fri, 10 Dec 2021, Bernhard Übelacker wrote:
Hello Francois, I tried to submit today this patch, unfortunately it timed out at each Windows 7 and 10 VMs.
It times out also with a patch that should not change behaviour when manually submitting. E.g. https://testbot.winehq.org/GetFile.pl?JobKey=103828
You should be able to diagnose that by checking:
Timestamp traces (WINETEST_TIME) and/or Report successful tests (WINETEST_REPORT_SUCCESS)
I assume in my case winetest detects the runtime of more than 120 seconds, terminates the child process and ends itself, then the screenshot is taken?
Usually yes. Technically there's a bit of a race between the timeout in Winetest.exe and the process monitoring the task on the TestBot server. But normally Winetest.exe is supposed to time out first.
It looks like the 'stack trace' of the test getting stuck is something like this:
itemdlg.c:1530:9.359 Test succeeded // test_filename() itemdlg.c:102:9.406 Test succeeded // get_hwnd_from_ifiledialog() itemdlg.c:221:9.422 Test succeeded // IFileDialogEvents_fnOnFolderChange()
https://testbot.winehq.org/JobDetails.pl?Key=103851
It may be necessary to add some more traces to pinpoint exactly where the test gets stuck.
Is there a way to get the screenshot before the timeout of winetest.exe is reached? E.g. a possibility to reduce some kind of "outer" timeout?
There's a plan to take screenshots at a to-be-determined regular interval. Some progress has been made on it but there's a dozen other things to improve on the TestBot and at some point it fell by the wayside :-(
Hello Francois, thanks for the pointers. I got an idea to run the test in a detached process, so it would not get killed... And it seems it worked: https://testbot.winehq.org/JobDetails.pl?Key=103923 The screenshot shows: Open winetest File not found. Check the file name and try again. OK And the last lines of the test output: itemdlg.c:1530:7.281 Test succeeded itemdlg.c:99:7.360 Test succeeded itemdlg.c:102:7.360 Test succeeded itemdlg.c:213:7.360 Test succeeded itemdlg.c:216:7.375 Test succeeded itemdlg.c:221:7.375 Test succeeded 1530:itemdlg:10.015 0 tests executed (0 marked as todo, 0 failures), 0 skipped. comdlg32:itemdlg:1530 done (0) in 10s The last two lines seem to be from the "parent" process, the lines above from the detached "child" process. So it looks kind of after the "OK command" in line 220 the dialog is not operating in the directory it got told to ... Attached the patch if it might be of help for future cases (the Sleep might need to get adjusted). Just another small note: I first tested this against the w7u_2qxl (103921), but there the screen stays empty, not even a taskbar. So I wonder if the screenshot get taken from the second screen. Thanks and Kind regards, Bernhard
Am 11.12.21 um 11:26 schrieb Bernhard Übelacker:
Am 10.12.21 um 16:42 schrieb Francois Gouget:
On Fri, 10 Dec 2021, Bernhard Übelacker wrote:
Hello Francois, I tried to submit today this patch, unfortunately it timed out at each Windows 7 and 10 VMs.
It times out also with a patch that should not change behaviour when manually submitting. E.g. https://testbot.winehq.org/GetFile.pl?JobKey=103828
You should be able to diagnose that by checking:
Timestamp traces (WINETEST_TIME) and/or Report successful tests (WINETEST_REPORT_SUCCESS)
I assume in my case winetest detects the runtime of more than 120 seconds, terminates the child process and ends itself, then the screenshot is taken?
Usually yes. Technically there's a bit of a race between the timeout in Winetest.exe and the process monitoring the task on the TestBot server. But normally Winetest.exe is supposed to time out first.
It looks like the 'stack trace' of the test getting stuck is something like this:
itemdlg.c:1530:9.359 Test succeeded // test_filename() itemdlg.c:102:9.406 Test succeeded // get_hwnd_from_ifiledialog() itemdlg.c:221:9.422 Test succeeded // IFileDialogEvents_fnOnFolderChange()
https://testbot.winehq.org/JobDetails.pl?Key=103851
It may be necessary to add some more traces to pinpoint exactly where the test gets stuck.
Is there a way to get the screenshot before the timeout of winetest.exe is reached? E.g. a possibility to reduce some kind of "outer" timeout?
There's a plan to take screenshots at a to-be-determined regular interval. Some progress has been made on it but there's a dozen other things to improve on the TestBot and at some point it fell by the wayside :-(
Hello Francois, thanks for the pointers. I got an idea to run the test in a detached process, so it would not get killed...
And it seems it worked: https://testbot.winehq.org/JobDetails.pl?Key=103923
The screenshot shows: Open winetest File not found. Check the file name and try again. OK
And the last lines of the test output: itemdlg.c:1530:7.281 Test succeeded itemdlg.c:99:7.360 Test succeeded itemdlg.c:102:7.360 Test succeeded itemdlg.c:213:7.360 Test succeeded itemdlg.c:216:7.375 Test succeeded itemdlg.c:221:7.375 Test succeeded 1530:itemdlg:10.015 0 tests executed (0 marked as todo, 0 failures), 0 skipped. comdlg32:itemdlg:1530 done (0) in 10s
The last two lines seem to be from the "parent" process, the lines above from the detached "child" process.
So it looks kind of after the "OK command" in line 220 the dialog is not operating in the directory it got told to ...
Attached the patch if it might be of help for future cases (the Sleep might need to get adjusted).
Just another small note: I first tested this against the w7u_2qxl (103921), but there the screen stays empty, not even a taskbar. So I wonder if the screenshot get taken from the second screen.
Thanks and Kind regards, Bernhard
Hello Francois, just a small addition. This hang with this "File not found" message seems to be caused by the test started with current directory in public documents, where also the test files get generated. Therefore cannot find them in my documents. This patch changes the current directory if found to be in public documents: https://source.winehq.org/patches/data/222000 This difference is also visible in my initial job 103829 in the task log, where failing runs got started from public documents. Kind regards, Bernhard
On Sat, 11 Dec 2021, Bernhard Übelacker wrote: [...]
This hang with this "File not found" message seems to be caused by the test started with current directory in public documents, where also the test files get generated.
While most of the TestBot VMs start the tests in public documents, they may start them in other places. One VM started them from e:\ (though that may have been a Vista VM so moot now). In any case the tests cannot make assumptions on what the current directory will be. So if comdlg32:itemdlg needs to be run in the user's Documents directory, then it should chdir there, no matter what the initial directory was.
Therefore cannot find them in my documents.
I did not go through the test code in detail so I may ask a stupid question. But why does the dialog look for the files in the user's documents folder? Is it because the dialog is opened with an option that says to show the content of the user's documents folder? If so then maybe the test files should be created there and then changing the current directory would not even be needed. Or maybe the dialog could be told to open the current directory instead, though that may be less reliable (e.g. is cwd == c:\ ) -- Francois Gouget <fgouget(a)codeweavers.com>
Am 11.12.21 um 19:41 schrieb Francois Gouget:
On Sat, 11 Dec 2021, Bernhard Übelacker wrote: [...]
This hang with this "File not found" message seems to be caused by the test started with current directory in public documents, where also the test files get generated.
While most of the TestBot VMs start the tests in public documents, they may start them in other places. One VM started them from e:\ (though that may have been a Vista VM so moot now).
In any case the tests cannot make assumptions on what the current directory will be. So if comdlg32:itemdlg needs to be run in the user's Documents directory, then it should chdir there, no matter what the initial directory was.
Therefore cannot find them in my documents.
I did not go through the test code in detail so I may ask a stupid question. But why does the dialog look for the files in the user's documents folder?
Is it because the dialog is opened with an option that says to show the content of the user's documents folder?
If so then maybe the test files should be created there and then changing the current directory would not even be needed.
Or maybe the dialog could be told to open the current directory instead, though that may be less reliable (e.g. is cwd == c:\ )
As far as I saw the test makes no assumptions about the path being started from. It queries the current directory [1], creates some file inside [2], and creates the file open dialog [3] and sets the initial directory to the path queried before (through an IShellItem object). Until now every path observed was C:\Users\Public\Documents, where the test was started from (at this VM). Then the OK is simulated [5]. If short before this point the dialog is queried (again via conversion through an IShellItem), then it returns C:\Users\winetest\Documents. I have tried to directly convert the IShellItem back to a real path, but there it returned the input from the beginning, the current directory. Also I have tried to change current directory at process start to just C:\Users\winetest, and there the dialog did not hang, all "open" dialogs showed this path. Therefore I assume that this is some special handling of the C:\Users\winetest\Documents / C:\Users\Public\Documents directories, at Microsoft side. I proposed already following patch, which changes current directory, in case we expect an issue: https://source.winehq.org/patches/data/222000 Otherwise it operates where it got started from. [1] https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/comdlg32/tests/itemdl... [2] https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/comdlg32/tests/itemdl... [3] https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/comdlg32/tests/itemdl... [4] https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/comdlg32/tests/itemdl... [5] https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/comdlg32/tests/itemdl... Kind regards, Bernhard
participants (3)
-
Bernhard Übelacker -
Francois Gouget -
Marvin