Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11156fcb32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (cacheFileInfo.Length > MaxCachedIconBytes) | ||
| { | ||
| StartupDiagnostics.Log($"LoadIconBytes ignored oversized cached icon for {processName}: {cacheFileInfo.Length} bytes."); | ||
| return null; |
There was a problem hiding this comment.
Returning here skips the executable-extraction fallback later in LoadIconBytes, so any oversized cache entry (and the zero-byte case just above) now leaves that app with no icon until the user manually clears %LocalAppData%\inputor. This is easy to hit with a partially written cache file or with icons cached before the new 1 MiB limit was introduced; before this change those cases still fell through to ExtractAssociatedIcon and rebuilt the cache.
Useful? React with 👍 / 👎.
| return null; | ||
| } | ||
|
|
||
| var diskBytes = File.ReadAllBytes(cacheFilePath); |
There was a problem hiding this comment.
This ReadAllBytes call still reads from a user-writable path after the size check happened on a separate FileInfo, so another local process can replace or grow the file between the two operations and force an unbounded allocation anyway. Because the commit’s goal is to prevent cache-file DoS, the read needs to stay on the same open handle and stop after MaxCachedIconBytes instead of reopening the path here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 658c0bf6da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static void TryPersistIconBytes(string? cacheFilePath, byte[] iconBytes, int generation) | ||
| { | ||
| if (cacheFilePath is null || iconBytes.Length == 0 || generation != IconCacheGeneration) | ||
| if (cacheFilePath is null || iconBytes.Length == 0 || iconBytes.Length > MaxCachedIconBytes || generation != IconCacheGeneration) |
There was a problem hiding this comment.
When iconBytes.Length > MaxCachedIconBytes, this guard skips writing anything to disk, but LoadIconBytes can only regenerate an icon from Process.GetProcessesByName(...) while that app is currently running. For any executable whose extracted PNG is larger than 1 MiB, the icon will appear during the current session but disappear after restart (or whenever the process is no longer running), so historical app rows permanently fall back to the generic glyph. If the intent is to bound the cache, we still need to persist a downsized/re-encoded icon instead of dropping the cache entry entirely.
Useful? React with 👍 / 👎.
1 participant
Motivation
LoadIconBytespreviously usedFile.ReadAllByteson files in the user-writable icon cache with no size checks, allowing a local attacker or corrupted cache to place an oversized file that could exhaust memory and cause a denial-of-service.Description
MaxCachedIconBytesconstant (1 MiB) and use it to bound cached icon sizes.LoadIconBytescheck theFileInfo.Lengthbefore reading and log & skip any cached files that are oversized or empty.TryPersistIconBytesrefuse to write icon blobs larger thanMaxCachedIconBytesto avoid populating the cache with oversized files.Testing
dotnet build inputor.slnbut the container does not have the .NET SDK installed, so the build could not be executed (dotnet: command not found).dotnet run --project src/inputor.WinUI/inputor.WinUI.csproj -- --count-sample "Hello世界"but it was blocked for the same reason; no automated tests were run because the repository contains no test project.Codex Task