Dependabot/nuget/system.io.hashing 10.0.1 #17962
Conversation
--- updated-dependencies: - dependency-name: System.IO.Hashing dependency-version: 10.0.1 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| public string GetRecentFolderPath() | ||
| { | ||
| using ComHeapPtr<char> pwszRecentFolderPath = default; | ||
| PInvoke.SHGetKnownFolderPath(FOLDERID.FOLDERID_Recent, KNOWN_FOLDER_FLAG.KF_FLAG_DONT_VERIFY | KNOWN_FOLDER_FLAG.KF_FLAG_NO_ALIAS, HANDLE.Null, (PWSTR*)pwszRecentFolderPath.GetAddressOf()); | ||
| return new(pwszRecentFolderPath.Get()); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| bool fRes = pszBuffer.Allocate(1024U); | ||
| if (!fRes) return null; | ||
|
|
||
| uint cchBuffer = PInvoke.GetEnvironmentVariable((PCWSTR)Unsafe.AsPointer(ref Unsafe.AsRef(in name.GetPinnableReference())), pszBuffer.Get(), (uint)name.Length + 1); |
There was a problem hiding this comment.
Bug: The GetEnvironmentVariable and ResolveIndirectString methods pass the input string length as the buffer size to Windows APIs, not the actual allocated buffer size of 1024 characters.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The GetEnvironmentVariable and ResolveIndirectString methods incorrectly calculate the buffer size passed to the underlying Windows APIs. A buffer of 1024 characters is allocated, but the size passed to PInvoke.GetEnvironmentVariable and PInvoke.SHLoadIndirectString is based on the input string's length. According to API documentation, this parameter should be the total capacity of the output buffer. If the resolved string is longer than the provided input string length, this could lead to a buffer overflow and memory corruption.
💡 Suggested Fix
In the calls to PInvoke.GetEnvironmentVariable and PInvoke.SHLoadIndirectString, replace the incorrect size argument (e.g., (uint)name.Length + 1) with the actual allocated buffer size, which is 1024U.
🤖 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/Helpers/WindowsStorageHelpers.System.cs#L20
Potential issue: The `GetEnvironmentVariable` and `ResolveIndirectString` methods
incorrectly calculate the buffer size passed to the underlying Windows APIs. A buffer of
1024 characters is allocated, but the size passed to `PInvoke.GetEnvironmentVariable`
and `PInvoke.SHLoadIndirectString` is based on the input string's length. According to
API documentation, this parameter should be the total capacity of the output buffer. If
the resolved string is longer than the provided input string length, this could lead to
a buffer overflow and memory corruption.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7515939
| if (_explorerADL is not null) ((IUnknown*)_explorerADL)->Release(); | ||
| if (_filesADL is not null) ((IUnknown*)_filesADL)->Release(); | ||
| if (_filesCDL is not null) ((IUnknown*)_filesCDL)->Release(); | ||
| } |
There was a problem hiding this comment.
Bug: The JumpListManager singleton leaks a COM object by failing to call Release() on the _filesICDL member in its Dispose method, leading to a resource leak.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The JumpListManager class, which is a long-lived singleton, allocates a COM object for _filesICDL via CoCreateInstance in its Create method. However, the corresponding Dispose method fails to call Release() on this object, while it correctly does so for three other COM objects. This violates COM reference counting rules, causing a resource leak that persists for the application's lifetime. The unreleased object will consume resources until the application exits.
💡 Suggested Fix
In the Dispose method of JumpListManager, add a line to release the _filesICDL COM object, similar to the other COM objects being released: if (_filesICDL is not null) ((IUnknown*)_filesICDL)->Release();.
🤖 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/Managers/JumpListManager.cs#L495-L498
Potential issue: The `JumpListManager` class, which is a long-lived singleton, allocates
a COM object for `_filesICDL` via `CoCreateInstance` in its `Create` method. However,
the corresponding `Dispose` method fails to call `Release()` on this object, while it
correctly does so for three other COM objects. This violates COM reference counting
rules, causing a resource leak that persists for the application's lifetime. The
unreleased object will consume resources until the application exits.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7515939
3 participants
Resolved / Related Issues
To prevent extra work, all changes to the Files codebase must link to an approved issue marked as
Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.Steps used to test these changes
Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.