[PATCH v2 0/1] MR7451: dlls/shell32: Fix IUnknown::QueryInterface() to meet spec
The spec says if the passed pointer is NULL then return E_POINTER and not to just unconditionally deref then set NULL. Signed-off-by: Edward O'Callaghan <edward(a)antitrust.cc> -- v2: dlls/shell32: Fix IUnknown::QueryInterface() to meet spec https://gitlab.winehq.org/wine/wine/-/merge_requests/7451
From: Edward O'Callaghan <edward(a)antitrust.cc> The spec says if the passed pointer is NULL then return E_POINTER and not to just unconditionally deref then set NULL. Signed-off-by: Edward O'Callaghan <edward(a)antitrust.cc> --- dlls/shell32/shelllink.c | 19 ++++++++++--------- dlls/shell32/tests/shelllink.c | 4 ++++ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/dlls/shell32/shelllink.c b/dlls/shell32/shelllink.c index 30cd038728f..9e18546b6af 100644 --- a/dlls/shell32/shelllink.c +++ b/dlls/shell32/shelllink.c @@ -1558,7 +1558,8 @@ static HRESULT WINAPI IShellLinkW_fnQueryInterface( TRACE("(%p)->(%s)\n", This, debugstr_guid(riid)); - *ppvObj = NULL; + if (!ppvObj) + return E_POINTER; if(IsEqualIID(riid, &IID_IUnknown) || IsEqualIID(riid, &IID_IShellLinkA)) { @@ -1595,16 +1596,16 @@ static HRESULT WINAPI IShellLinkW_fnQueryInterface( else if(IsEqualIID(riid, &IID_IPropertyStore)) { *ppvObj = &This->IPropertyStore_iface; + } else { + *ppvObj = NULL; + ERR("-- Interface: E_NOINTERFACE\n"); + return E_NOINTERFACE; } - if(*ppvObj) - { - IUnknown_AddRef((IUnknown*)*ppvObj); - TRACE("-- Interface: (%p)->(%p)\n", ppvObj, *ppvObj); - return S_OK; - } - ERR("-- Interface: E_NOINTERFACE\n"); - return E_NOINTERFACE; + IUnknown_AddRef((IUnknown*)*ppvObj); + TRACE("-- Interface: (%p)->(%p)\n", ppvObj, *ppvObj); + + return S_OK; } /****************************************************************************** diff --git a/dlls/shell32/tests/shelllink.c b/dlls/shell32/tests/shelllink.c index f13155ade2f..2715733af93 100644 --- a/dlls/shell32/tests/shelllink.c +++ b/dlls/shell32/tests/shelllink.c @@ -887,6 +887,10 @@ static void test_datalink(void) r = IShellLinkW_QueryInterface( sl, &IID_IShellLinkDataList, (void **)&dl ); ok( r == S_OK, "Failed to get interface, hr %#lx.\n", r); + /* test null obj case */ + r = IShellLinkW_QueryInterface( sl, &IID_IShellLinkDataList, NULL ); + ok(r == E_POINTER, "Unexpected hr %#lx.\n", r); + flags = 0; r = IShellLinkDataList_GetFlags( dl, &flags ); ok( r == S_OK, "GetFlags failed\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7451
On Sat Mar 1 22:47:55 2025 +0000, Nikolay Sivov wrote:
This needs a test. Continuous disintegration test added.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96335
Please also mention what application requires this. We don't change things just to "follow the spec", particularly since there's no such thing as a spec for the Windows API. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96474
On Sun Mar 2 23:43:14 2025 +0000, Alexandre Julliard wrote:
Please also mention what application requires this. We don't change things just to "follow the spec", particularly since there's no such thing as a spec for the Windows API. Cashbooks. Ay? The specs I am referring to is that, (1) in C you cannot just deref a pointer unconditionally and (2) the MS doc [says as such](https://learn.microsoft.com/en-us/windows/win32/api/unknwn/nf-unknwn-iunknow...)) "If ppvObject (the address) is nullptr, then this method returns E_POINTER.". Why is every change purposed to this project meet with such adversarial attitude?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96478
On Sun Mar 2 23:43:14 2025 +0000, Edward OC wrote:
Cashbooks. Ay? The specs I am referring to is that, (1) in C you cannot just deref a pointer unconditionally and (2) the MS doc [says as such](https://learn.microsoft.com/en-us/windows/win32/api/unknwn/nf-unknwn-iunknow...)) "If ppvObject (the address) is nullptr, then this method returns E_POINTER.". Why is every change purposed to this project meet with such adversarial attitude? Every change has a cost, in code churn, in reviewer's time, in this case also in extra runtime checks.
Since we don't have infinite resources, we have to concentrate on changes that actually further our goal of running existing Windows applications. There's an endless supply of things that could be changed, but most of them don't make a difference for actual apps, so we don't change them. I'm sorry if it sounds adversarial, but since you are a newcomer, we are trying to make our approach clear right away, so that you don't waste your time (and ours) by, say, trying to "fix" every single QueryInterface call. If you want to help, the right way is to find an application that you want to run, try it, look at what doesn't work, and then submit the changes that are necessary to fix these specific bugs. If you are sending a change and you can't explain what application it is helping, that's usually a sign that you shouldn't be making that change. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96527
On Mon Mar 3 11:13:51 2025 +0000, Alexandre Julliard wrote:
Every change has a cost, in code churn, in reviewer's time, in this case also in extra runtime checks. Since we don't have infinite resources, we have to concentrate on changes that actually further our goal of running existing Windows applications. There's an endless supply of things that could be changed, but most of them don't make a difference for actual apps, so we don't change them. I'm sorry if it sounds adversarial, but since you are a newcomer, we are trying to make our approach clear right away, so that you don't waste your time (and ours) by, say, trying to "fix" every single QueryInterface call. If you want to help, the right way is to find an application that you want to run, try it, look at what doesn't work, and then submit the changes that are necessary to fix these specific bugs. If you are sending a change and you can't explain what application it is helping, that's usually a sign that you shouldn't be making that change. Since there seem to be multiple apps named "Cashbooks," mind linking to its homepage or vendor? In any case, thanks for your work on improving Wine's compatibility!
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96529
On Mon Mar 3 11:14:54 2025 +0000, Jinoh Kang wrote:
Since there seem to be multiple apps named "Cashbooks," mind linking to its homepage or vendor? In any case, thanks for your work on improving Wine's compatibility! @iamahuman , first person to say thank you, appreciate that!
I had made a db entry here https://appdb.winehq.org/objectManager.php?sClass=version&iId=42441 and linked the bug here https://bugs.winehq.org/show_bug.cgi?id=57662 previously. The db entry [links the bin](https://www.acclaimsoftware.com.au/download/). The application relies heavily on oleaut32 being correct, which it isn't even close to. The vast majority of my merge requests have been work around that. Unfortunately, all so far have been subject to either drive-by "reviews" and then subsequent ignores or responses similar to the above. The issue with the philosophy of the one stated by @julliard is that "resources" will remain small if you say "don't change other peoples code" (in another merge request last week) because that has a direct mapping to telling new developers to basically **** off. These "newcomers" can contribute and help alleviate resource constraint that frankly everyone who is a adult suffers from. Working with people improves that. On the technical side, fixing code that has the potential to crash in trivially obvious ways, violates documented behavior because it may not have an immediate effect on _your_ computer Alexandre isn't really a logical argument in the slightest. Such changes hardly amount to 'churn' that directly fix segmentation faults. Additionally I sent in a patch last week that fixes a regression made to oleaut32 that occured in the last couple of weeks, totally ignored. Typically fixing memory corruption issues and consolidating common paths into helpers written consistently usually helps people reach their "goals" faster. The only thing this "straight forward, no churn we have limited resources" messaging communicate to developers outside your close knit circle is a toxic attitude. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96535
On Mon Mar 3 11:37:25 2025 +0000, Edward OC wrote:
@iamahuman , first person to say thank you, appreciate that! I had made a db entry here https://appdb.winehq.org/objectManager.php?sClass=version&iId=42441 and linked the bug here https://bugs.winehq.org/show_bug.cgi?id=57662 previously. The db entry [links the bin](https://www.acclaimsoftware.com.au/download/). The application relies heavily on oleaut32 being correct, which it isn't even close to. The vast majority of my merge requests have been work around that. Unfortunately, all so far have been subject to either drive-by "reviews" and then subsequent ignores or responses similar to the above. The issue with the philosophy of the one stated by @julliard is that "resources" will remain small if you say "don't change other peoples code" (in another merge request last week) because that has a direct mapping to telling new developers to basically **** off. These "newcomers" can contribute and help alleviate resource constraint that frankly everyone who is a adult suffers from. Working with people improves that. On the technical side, fixing code that has the potential to crash in trivially obvious ways, violates documented behavior because it may not have an immediate effect on _your_ computer Alexandre isn't really a logical argument in the slightest. Such changes hardly amount to 'churn' that directly fix segmentation faults. Additionally I sent in a patch last week that fixes a regression made to oleaut32 that occured in the last couple of weeks, totally ignored. Typically fixing memory corruption issues and consolidating common paths into helpers written consistently usually helps people reach their "goals" faster. The only thing this "straight forward, no churn we have limited resources" messaging communicate to developers outside your close knit circle is a toxic attitude. btw "extra runtime checks" on validating user controlled pointers to a API is a completely irreverent nano-optimisation, CPU's have branch-predictors. Fixing the oleaut32 implementation from O(n^3) strcmp() lookups to a hashmap, which is how it is meant to work, is far more relevant. If you are worried about branch-complexity, the other patches I sent in that reduce cyclomatic branch complexity help the `if (true) { if (true) { if (true` cascades because base-conditions are not quickly interrogated and returned from.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96537
On Mon Mar 3 11:43:45 2025 +0000, Edward OC wrote:
btw "extra runtime checks" on validating user controlled pointers to a API is a completely irreverent nano-optimisation, CPU's have branch-predictors. Fixing the oleaut32 implementation from O(n^3) strcmp() lookups to a hashmap, which is how it is meant to work, is far more relevant. If you are worried about branch-complexity, the other patches I sent in that reduce cyclomatic branch complexity help the `if (true) { if (true) { if (true` cascades because base-conditions are not quickly interrogated and returned from. "Validating" pointers doesn't fix anything, it only serves to hide bugs. Yes, Microsoft does add code to hide bugs in many cases, which is why Windows apps are so broken.
In Wine we want to make sure that the bugs get fixed, so we don't hide them unless it's necessary. There's nothing useful to gain by calling QueryInterface with a NULL pointer, so if an app does this, it's usually a sign that something else went wrong. That's what needs to be investigated and fixed, instead of hiding the bug.
Additionally I sent in a patch last week that fixes a regression made to oleaut32 that occured in the last couple of weeks, totally ignored.
I haven't seen any patch of yours that mentioned fixing a regression, much less which regression. I've seen a dozen oleaut32 patches that are mostly moving code around and renaming variables, it's impossible to tell if there was a valid bug fix buried in there. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96538
On Mon Mar 3 12:05:51 2025 +0000, Alexandre Julliard wrote:
"Validating" pointers doesn't fix anything, it only serves to hide bugs. Yes, Microsoft does add code to hide bugs in many cases, which is why Windows apps are so broken. In Wine we want to make sure that the bugs get fixed, so we don't hide them unless it's necessary. There's nothing useful to gain by calling QueryInterface with a NULL pointer, so if an app does this, it's usually a sign that something else went wrong. That's what needs to be investigated and fixed, instead of hiding the bug.
Additionally I sent in a patch last week that fixes a regression made to oleaut32 that occured in the last couple of weeks, totally ignored. I haven't seen any patch of yours that mentioned fixing a regression, much less which regression. I've seen a dozen oleaut32 patches that are mostly moving code around and renaming variables, it's impossible to tell if there was a valid bug fix buried in there. Validating does not "hide bugs", you are just making stuff up now. The application expects to have `E_POINTER` upon a `NULL` passed in. The reason why a `NULL` was passed is irrelevant, only that a specific state is expected. You have no idea what the application is doing nor why, only that it is a matter of fact that this is the behavior expected from the API. Since we cannot control the application code we can only provide it with what it expects and fix bugs pertaining to that. Incidentally your reasoning about Microsoft application being buggy is nonsensical, it has nothing to do with API's validating arguments - a blindly obvious practice in software engineering - it has to do with their huge API surface inherits vast amounts of complexity and from which issue emerges.
`12c79c33a1d9c` was a regression, looks like it was fixed now by someone else. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96543
The application relies heavily on oleaut32 being correct, which it isn't even close to. The vast majority of my merge requests have been work around that.
I understand. As someone who had to fix COM-related race bugs, I largely agree that the current state of COM/OLE-related libraries isn't exactly ideal. Wine's primary goal, first and foremost, is to maximize compatibility with Windows apps, even if it doesn't perfectly emulate Windows. If a simple stub is enough to unblock execution, we do that. If a partial implementation gets it forward without breaking other apps, it's good enough. Given enough time we could "get everything right" the first time, but trust me, with hundreds and thousands of Windows DLLs (Wine already has ~700 of them!) we really need some prioritization to match an operating system developed by a multi-national corporation. Wine itself is about incremental changes, rather than a clearly defined roadmap. It's expected that cruft accumulates over time. Sometimes, maintainers can find time to clean them up by themselves. Other times, it's expected of contributors to take up the work. After all, once it's accepted and merged, it's the maintainers' job now to keep it working and avoid breaking other paps.
Validating does not "hide bugs", you are just making stuff up now. The application expects to have `E_POINTER` upon a `NULL` passed in. The reason why a `NULL` was passed is irrelevant, only that a specific state is expected. You have no idea what the application is doing nor why, only that it is a matter of fact that this is the behavior expected from the API. Since we cannot control the application code we can only provide it with what it expects and fix bugs pertaining to that. Incidentally your reasoning about Microsoft application being buggy is nonsensical, it has nothing to do with API's validating arguments - a blindly obvious practice in software engineering - it has to do with their huge API surface inherits vast amounts of complexity and from which issue emerges.
Make no mistake, we are aware of apps that rely on error handling behavior and NULL pointer checks (just look for `E_POINTER` in wine codebase). In absence of an explicit evidence of an app relying on this particular behavior, we assume that the source of the "invalid pointer" bug lies elsewhere. Not fixing the root cause has *historically* been a common source of error from new contributors, so you'll probably encounter this default objection unless you're explicit about the error-check reliant behavior from the application side. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96544
On Mon Mar 3 12:36:12 2025 +0000, Jinoh Kang wrote:
The application relies heavily on oleaut32 being correct, which it isn't even close to. The vast majority of my merge requests have been work around that. I understand. As someone who had to fix COM-related race bugs, I largely agree that the current state of COM/OLE-related libraries isn't exactly ideal. Wine's primary goal, first and foremost, is to maximize compatibility with Windows apps, even if it doesn't perfectly emulate Windows. If a simple stub is enough to unblock execution, we do that. If a partial implementation gets it forward without breaking other apps, it's good enough. Given enough time we could "get everything right" the first time, but trust me, with hundreds and thousands of Windows DLLs (Wine already has ~700 of them!) we really need some prioritization to match an operating system developed by a multi-national corporation. Wine itself is about incremental changes, rather than a clearly defined roadmap. It's expected that cruft accumulates over time. Sometimes, maintainers can find time to clean them up by themselves. Other times, it's expected of contributors to take up the work. After all, once it's accepted and merged, it's the maintainers' job now to keep it working and avoid breaking other apps. See https://docs.google.com/presentation/d/11rMc8PBeyMItV6hv31OHSZ626_6FCZxjX6Zx... (from Linux kernel community) for more on this. Validating does not "hide bugs", you are just making stuff up now. The application expects to have `E_POINTER` upon a `NULL` passed in. The reason why a `NULL` was passed is irrelevant, only that a specific state is expected. You have no idea what the application is doing nor why, only that it is a matter of fact that this is the behavior expected from the API. Since we cannot control the application code we can only provide it with what it expects and fix bugs pertaining to that. Incidentally your reasoning about Microsoft application being buggy is nonsensical, it has nothing to do with API's validating arguments - a blindly obvious practice in software engineering - it has to do with their huge API surface inherits vast amounts of complexity and from which issue emerges. Make no mistake, we are aware of apps that rely on error handling behavior and NULL pointer checks (just look for `E_POINTER` in wine codebase). In absence of an explicit evidence of an app relying on this particular behavior, we assume that the source of the "invalid pointer" bug lies elsewhere. Not fixing the root cause has *historically* been a common source of error from new contributors, so you'll probably encounter this default objection unless you're explicit about the error-check reliant behavior from the application side. @iamahuman thanks mate. I can see what your underlying reasoning is about people doing drive-by patches that fix a seg fault that is downstream for something else and you rather the application crash. However it is sort of a user facing behavior of crashing to track a bug report of the upstream issue that does not really generalise since other applications that expect `E_POINTER` benefit. This is obvious defined behavior.
I sense the underlying issue below this is that you have got these drive-by one liner patches before without a deeper understanding on how to investigate and contribute to wine more long term, hence you linked me to how kern dev is done. I assure you this isn't my first rodeo. Working on radeon graphics, the driver is half the size of the kernel, mesa3d is very complex and working on coreboot things are sometimes very hard to debug when you don't yet have a DRAM controller. Yes, wine has a big job but that is how software is today but we just get on with it. This patch was ultimately broken off from the rest of my larger bodies of work due to the review behaviors in order to puck out something least controversial to build momentum. It seems pretty clear to me now that upstream is a wast of my time. Comments like,
I've seen a dozen oleaut32 patches that are mostly moving code around and renaming variables, it's impossible to tell if there was a valid bug fix buried in there.
shows a lack of reciprocating investment to improve things. idk if some specific wine devs got burnt in the past from lots of drive-by patches that don't fix things so now have overcompensated and ignore new developers. The reality is that the oleaut32 implementation as-is in-tree is in a unworkable state to morph it into something actually functional. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7451#note_96552
participants (4)
-
Alexandre Julliard (＠julliard) -
Edward O'Callaghan
-
Edward OC (＠funfunctor)
-
Jinoh Kang (＠iamahuman)