Re: [PATCH 1/3] shlwapi: Be less strict on which type of IShellFolder can be enumerated
On 6/3/2010 23:35, Andrew Eikum wrote:
This fixes a bug with Internet Explorer 6's Favorites menu.
This function used to be named IUnknown_EnumObjects, so my guess is that the original implementor thought it'd be wise to verify the type of the object before executing IShellFolder's methods on it. Since it's now called IShellFolder_EnumObjects and explicitly takes an LPSHELLFOLDER parameter, I see no reason to do the typecheck.
And where is a test for that?
--- dlls/shlwapi/ordinal.c | 19 +------------------ dlls/shlwapi/tests/Makefile.in | 2 +- dlls/shlwapi/tests/ordinal.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 19 deletions(-)
On 06/03/2010 02:40 PM, Nikolay Sivov wrote:
On 6/3/2010 23:35, Andrew Eikum wrote:
This fixes a bug with Internet Explorer 6's Favorites menu.
This function used to be named IUnknown_EnumObjects, so my guess is that the original implementor thought it'd be wise to verify the type of the object before executing IShellFolder's methods on it. Since it's now called IShellFolder_EnumObjects and explicitly takes an LPSHELLFOLDER parameter, I see no reason to do the typecheck.
And where is a test for that?
In the patch. The object returned by SHGetDesktopFolder does not have CLSID_ShellFSFolder, yet it does not fail in the shlwapi EnumObjects call. Running the test with and without the functionality change shows this. I guess I could go try to dig up some IShellFolder instance which succeeds a plain IShellFolder_EnumObjects call and fails a shlwapi EnumObjects call, but I wouldn't even know where to start for that. If the old functionality was required, it needs a test or at least a comment. As far as current knoweldge goes, it's both simpler and more correct with this patch than it was without. Andrew
On 6/4/2010 00:00, Andrew Eikum wrote:
On 06/03/2010 02:40 PM, Nikolay Sivov wrote:
On 6/3/2010 23:35, Andrew Eikum wrote:
This fixes a bug with Internet Explorer 6's Favorites menu.
This function used to be named IUnknown_EnumObjects, so my guess is that the original implementor thought it'd be wise to verify the type of the object before executing IShellFolder's methods on it. Since it's now called IShellFolder_EnumObjects and explicitly takes an LPSHELLFOLDER parameter, I see no reason to do the typecheck.
And where is a test for that?
In the patch. The object returned by SHGetDesktopFolder does not have CLSID_ShellFSFolder, yet it does not fail in the shlwapi EnumObjects call. Running the test with and without the functionality change shows this.
I guess I could go try to dig up some IShellFolder instance which succeeds a plain IShellFolder_EnumObjects call and fails a shlwapi EnumObjects call, but I wouldn't even know where to start for that. If the old functionality was required, it needs a test or at least a comment. As far as current knoweldge goes, it's both simpler and more correct with this patch than it was without. No, this is not what I meant actually. The possible reason it was named as IUnknown_EnumObjects is the same as for the rest of similar calls. IUknown is a valid input for all of them and another _QueryInterface is performed for IID_IShellFolder is case of this function. That makes no sense to have such export as your patch makes it, but it's possible of course. I expect that is does check that IShellFolder is supported.
Andrew
On 06/03/2010 03:12 PM, Nikolay Sivov wrote:
No, this is not what I meant actually. The possible reason it was named as IUnknown_EnumObjects is the same as for the rest of similar calls. IUknown is a valid input for all of them and another _QueryInterface is performed for IID_IShellFolder is case of this function. That makes no sense to have such export as your patch makes it, but it's possible of course. I expect that is does check that IShellFolder is supported.
Okay, I see what you're saying now. I hadn't thought of that. After some quick tests, I think you're on the right path. I'll work on fixing it. Thanks! Andrew
On 06/03/2010 03:47 PM, Andrew Eikum wrote:
On 06/03/2010 03:12 PM, Nikolay Sivov wrote:
No, this is not what I meant actually. The possible reason it was named as IUnknown_EnumObjects is the same as for the rest of similar calls. IUknown is a valid input for all of them and another _QueryInterface is performed for IID_IShellFolder is case of this function. That makes no sense to have such export as your patch makes it, but it's possible of course. I expect that is does check that IShellFolder is supported.
Okay, I see what you're saying now. I hadn't thought of that. After some quick tests, I think you're on the right path. I'll work on fixing it. Thanks!
So, with this in mind, here are some much more extensive tests. As is explained in the comment, Windows _does_ get the ClassID of the object in this call. But, Windows crashes when a non-IShellFolder object is given to this function. Presumably it's trying to call IShellFolder_EnumObjects on it. So it must not be using the CLSID for this kind of type-checking. Trouble is, I have no idea what it is used for. I tried testing most of the CLSIDs in shlguid.h with different combinations of flags, none of which had any effect. It must just be looking out for a small number of specific CLSIDs. One possible guess is that it tweaks the flags parameter when certain CLSIDs are detected, but none of my tests turned up anything interesting. Any thoughts? Should I just send as-is? Andrew
On 6/4/2010 22:13, Andrew Eikum wrote:
On 06/03/2010 03:47 PM, Andrew Eikum wrote:
On 06/03/2010 03:12 PM, Nikolay Sivov wrote:
No, this is not what I meant actually. The possible reason it was named as IUnknown_EnumObjects is the same as for the rest of similar calls. IUknown is a valid input for all of them and another _QueryInterface is performed for IID_IShellFolder is case of this function. That makes no sense to have such export as your patch makes it, but it's possible of course. I expect that is does check that IShellFolder is supported.
Okay, I see what you're saying now. I hadn't thought of that. After some quick tests, I think you're on the right path. I'll work on fixing it. Thanks!
So, with this in mind, here are some much more extensive tests. As is explained in the comment, Windows _does_ get the ClassID of the object in this call. But, Windows crashes when a non-IShellFolder object is given to this function. Presumably it's trying to call IShellFolder_EnumObjects on it. So it must not be using the CLSID for this kind of type-checking. And if pointer doesn't support IPersist does it crash too?
Trouble is, I have no idea what it is used for. I tried testing most of the CLSIDs in shlguid.h with different combinations of flags, none of which had any effect. It must just be looking out for a small number of specific CLSIDs. One possible guess is that it tweaks the flags parameter when certain CLSIDs are detected, but none of my tests turned up anything interesting.
Any thoughts? Should I just send as-is? So it doesn't care about classid returned from IPersist. I think it's enough now to add tests for not-called QI for IShellFolder and a test for a bunch of known CLSIDs for folders. After that (if you dummy CLSID is allowed too) probably it's ok to remove all checks.
Andrew
On 6/4/2010 22:13, Andrew Eikum wrote:
On 06/03/2010 03:47 PM, Andrew Eikum wrote: So, with this in mind, here are some much more extensive tests. As is explained in the comment, Windows _does_ get the ClassID of the object in this call. But, Windows crashes when a non-IShellFolder object is given to this function. Presumably it's trying to call IShellFolder_EnumObjects on it. So it must not be using the CLSID for this kind of type-checking. And if pointer doesn't support IPersist does it crash too?
Yes. The test where QueryInterface_fail is TRUE tests this condition. It always returns E_NOINTERFACE in QI, yet EnumObjects is still called.
Trouble is, I have no idea what it is used for. I tried testing most of the CLSIDs in shlguid.h with different combinations of flags, none of which had any effect. It must just be looking out for a small number of specific CLSIDs. One possible guess is that it tweaks the flags parameter when certain CLSIDs are detected, but none of my tests turned up anything interesting.
Any thoughts? Should I just send as-is? So it doesn't care about classid returned from IPersist. I think it's enough now to add tests for not-called QI for IShellFolder and a test for a bunch of known CLSIDs for folders. After that (if you dummy CLSID is allowed too) probably it's ok to remove all checks.
I'm not sure I follow. As the tests in the patch show, QI is called up to two times, once for IPersist and once for IPersistFolder if the first fails. I can certainly add tests for returning different CLSIDs, though that seems uninteresting to me since they all behave identically. Removing the code in shlwapi.404 will lead to test failures since Windows _does_ make QI and GetClassID calls, it's just unknown what it does with the data it gets. What do you think? Remove the tests? Add todo_wine and remove the 'dummy' code in shlwapi.404? Thanks for your feedback, Andrew
On 6/5/2010 19:09, Andrew Eikum wrote:
Trouble is, I have no idea what it is used for. I tried testing most of the CLSIDs in shlguid.h with different combinations of flags, none of which had any effect. It must just be looking out for a small number of specific CLSIDs. One possible guess is that it tweaks the flags parameter when certain CLSIDs are detected, but none of my tests turned up anything interesting.
Any thoughts? Should I just send as-is?
So it doesn't care about classid returned from IPersist. I think it's enough now to add tests for not-called QI for IShellFolder and a test for a bunch of known CLSIDs for folders. After that (if you dummy CLSID is allowed too) probably it's ok to remove all checks.
I'm not sure I follow. As the tests in the patch show, QI is called up to two times, once for IPersist and once for IPersistFolder if the first fails. I can certainly add tests for returning different CLSIDs, though that seems uninteresting to me since they all behave identically.
I mean, create different types of shellfolders, then test that function accepts them all. And a simple check that QI isn't called for IShellFolder, so argument expected to be IShellFolder pointer.
Removing the code in shlwapi.404 will lead to test failures since Windows _does_ make QI and GetClassID calls, it's just unknown what it does with the data it gets.
What do you think? Remove the tests? Add todo_wine and remove the 'dummy' code in shlwapi.404?
Remove tests and add a note in function code that this GetClassID method is actually called for unknown purpose. Another way is to make is accept another CLSID, if I got it right it is why you started to patch it.
Thanks for your feedback, Andrew
On 6/5/2010 19:09, Andrew Eikum wrote:
I'm not sure I follow. As the tests in the patch show, QI is called up to two times, once for IPersist and once for IPersistFolder if the first fails. I can certainly add tests for returning different CLSIDs, though that seems uninteresting to me since they all behave identically.
I mean, create different types of shellfolders, then test that function accepts them all. And a simple check that QI isn't called for IShellFolder, so argument expected to be IShellFolder pointer.
Something like this, then?
On 6/5/2010 19:09, Andrew Eikum wrote:
I'm not sure I follow. As the tests in the patch show, QI is called up to two times, once for IPersist and once for IPersistFolder if the first fails. I can certainly add tests for returning different CLSIDs, though that seems uninteresting to me since they all behave identically.
I mean, create different types of shellfolders, then test that function accepts them all. And a simple check that QI isn't called for IShellFolder, so argument expected to be IShellFolder pointer.
Argh, webmail clients are frustrating. Here's the patch.
participants (2)
-
Andrew Eikum -
Nikolay Sivov