Code Quality: Sync the jump list with Explorer#17564
Open
0x5bfa wants to merge 21 commits intofiles-community:mainfrom
Open
Code Quality: Sync the jump list with Explorer#175640x5bfa wants to merge 21 commits intofiles-community:mainfrom
0x5bfa wants to merge 21 commits intofiles-community:mainfrom
Conversation
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
71afbb3 to
9eb7ae9
Compare
0x5bfa
changed the title
yair100
reviewed
Sep 7, 2025
yair100
reviewed
Sep 7, 2025
yair100
reviewed
Sep 7, 2025
yair100
reviewed
Sep 7, 2025
yair100
reviewed
Sep 7, 2025
yair100
force-pushed
the
main
branch
2 times, most recently
from
97999e5 to
806f922
Compare
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
3 times, most recently
from
278a971 to
33efe98
Compare
0x5bfa
changed the title
Member
|
This might help with #14526 |
Member
|
Would this fix #17659 |
Member
Member
Author
|
Yes. This is one of the motivation. (I updated the PR to include this) |
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
a038857 to
3669604
Compare
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
9daefd6 to
032a523
Compare
yair100
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
7529df0 to
ec45efa
Compare
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
2 times, most recently
from
edbda5e to
816fcdc
Compare
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
77c2c7a to
e236a0d
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if (!fRes) return null; | ||
|
|
||
| fixed (char* pSource = source) | ||
| hr = PInvoke.SHLoadIndirectString(pSource, pszBuffer.Get(), 1024U, null); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
ad2854f to
2326930
Compare
Comment on lines
+56
to
+60
| public bool Allocate(nuint cch) | ||
| { | ||
| _ptr = (T*)NativeMemory.Alloc(cch); // malloc() | ||
| return _ptr is not null; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
+66
to
+70
| public WindowsStorable() | ||
| { | ||
| void* globalInterfaceTable; | ||
| PInvoke.CoCreateInstance(CLSID.CLSID_StdGlobalInterfaceTable, null, CLSCTX.CLSCTX_INPROC_SERVER, IID.IID_IGlobalInterfaceTable, &globalInterfaceTable); | ||
| _globalInterfaceTable = (IGlobalInterfaceTable*)globalInterfaceTable; |
There was a problem hiding this comment.
Bug: The CoCreateInstance call in the WindowsStorable constructor does not check the returned HRESULT, which can lead to a null pointer dereference if the call fails.
Severity: HIGH
Suggested Fix
Check the HRESULT returned by PInvoke.CoCreateInstance. If it indicates failure, throw an exception to prevent the object from being created in an invalid state. This aligns with the error handling pattern used for similar COM object creations elsewhere in the repository, such as using .ThrowIfFailedOnDebug().
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/Files.App.Storage/Windows/WindowsStorable.cs#L66-L70
Potential issue: The constructor for `WindowsStorable` calls `PInvoke.CoCreateInstance`
to create a `GlobalInterfaceTable` but does not check the returned HRESULT for failure.
If this call fails, for instance due to low system resources or COM initialization
issues, the `_globalInterfaceTable` field will be an invalid or null pointer. Subsequent
access to properties like `ThisPtr` or `ContextMenu` will attempt to dereference this
pointer without a null check, leading to a null pointer dereference and an application
crash. This is inconsistent with other parts of the codebase where `CoCreateInstance`
results are always checked.
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
7402495 to
c57914d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolved / Related Issues
TODOs
Steps used to test these changes