fix: code audit — BSDIFF overflow, Zip traversal, ProcessExit deadlock, Trace listener, Strategy lifecycle, HubConnection, RetryPolicy#515
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a broad set of audit-driven fixes across GeneralUpdate.Core and GeneralUpdate.Differential (plus related test coverage), targeting correctness, safety (zip extraction), lifecycle reliability, and retry behavior.
Changes:
- Hardens BSDIFF/HDIFF patch read/write and adds edge-case regression tests.
- Fixes updater lifecycle/exit behavior and improves robustness around tracing, SignalR connection disposal, and retry classification.
- Addresses multiple filesystem/zip/path handling issues and adds regression tests around patch pipelines and file-tree comparisons.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/DrivelutionTest/Utilities/VersionComparerAndRestartHelperTests.cs | Adds overflow-focused SemVer comparison tests for very large numeric components/prerelease identifiers. |
| tests/DifferentialTest/Differ/BsdiffDifferTests.cs | Adds BSDIFF edge-case round-trip tests (tiny files, repeating patterns, single-byte changes). |
| tests/DifferentialTest/ComprehensiveDifferentialTests.cs | Adds regression tests ensuring unknown files copy preserves subdirectories and supports independent app/patch paths. |
| tests/CoreTest/Strategy/StrategyCreationTests.cs | Adds regression test ensuring chained versions get distinct per-version patch subdirectories. |
| tests/CoreTest/FileSystem/FileTreeTests.cs | Adds null/imbalance behavior tests and documents existing leaf-node compare behavior. |
| tests/CoreTest/FileSystem/FileNodeTests.cs | Updates expectation for Equals() behavior with non-FileNode inputs. |
| src/c#/GeneralUpdate.Extension/Core/GeneralExtensionHost.cs | Adjusts lifecycle hook metadata ID/name extraction from extension zip filename. |
| src/c#/GeneralUpdate.Drivelution/Core/Utilities/VersionComparer.cs | Switches SemVer numeric parsing to long + overflow-aware prerelease comparison. |
| src/c#/GeneralUpdate.Differential/Differ/StreamingHdiffDiffer.cs | Prevents silent truncation in file reads; adjusts sign-magnitude write handling. |
| src/c#/GeneralUpdate.Differential/Differ/BsdiffDiffer.cs | Adds additional guards in patch application (size/seek bounds), sentinel handling, and sign-magnitude write handling. |
| src/c#/GeneralUpdate.Core/Tracer/GeneralTracer.cs | Tracks owned listeners to avoid clearing other libraries’ listeners on Dispose (but still clears on static init). |
| src/c#/GeneralUpdate.Core/Strategy/WindowsStrategy.cs | Avoids unconditional updater termination when app launch fails (gates exit on successful launch). |
| src/c#/GeneralUpdate.Core/Strategy/OssStrategy.cs | Improves cross-platform upgrade executable name selection and caps diagnostic file listing. |
| src/c#/GeneralUpdate.Core/Strategy/MacStrategy.cs | Adjusts exit/termination logic (still exits even when no app to launch). |
| src/c#/GeneralUpdate.Core/Strategy/LinuxStrategy.cs | Avoids unconditional updater termination when app launch fails (gates exit on successful launch). |
| src/c#/GeneralUpdate.Core/Strategy/ClientStrategy.cs | Avoids ProcessExit deadlock risk by offloading async hook; hardens version parsing in CheckFail. |
| src/c#/GeneralUpdate.Core/Strategy/AbstractStrategy.cs | Uses per-version patch subdirectories under a shared patch root to avoid chain-package overwrites. |
| src/c#/GeneralUpdate.Core/Pipeline/DiffPipeline.cs | Disposes semaphores, optimizes delete-manifest matching, and reworks CopyUnknownFiles relative path handling. |
| src/c#/GeneralUpdate.Core/Network/VersionService.cs | Makes global SSL policy field volatile for visibility across threads. |
| src/c#/GeneralUpdate.Core/Hubs/UpgradeHubService.cs | Adds disposed-state handling and nulls connection after DisposeAsync to prevent NREs. |
| src/c#/GeneralUpdate.Core/FileSystem/StorageManager.cs | Adds warning logs when file enumeration fails instead of swallowing exceptions silently. |
| src/c#/GeneralUpdate.Core/FileSystem/FileTree.cs | Adds null-safe traversal using null-conditionals in Compare(). |
| src/c#/GeneralUpdate.Core/FileSystem/FileNode.cs | Changes Equals(object) to return false instead of throwing for non-FileNode objects. |
| src/c#/GeneralUpdate.Core/Event/EventManager.cs | Adds Reset() and snapshots invocation list to avoid delegate mutation during dispatch. |
| src/c#/GeneralUpdate.Core/Download/Policy/DefaultRetryPolicy.cs | Adds StatusCode-based retry checks + regex fallback for 5xx codes. |
| src/c#/GeneralUpdate.Core/Differential/DefaultDirtyMatcher.cs | Removes redundant filename suffix stripping in patch-file matching. |
| src/c#/GeneralUpdate.Core/Configuration/ProcessContract.cs | Fixes null-check ordering for installPath validation. |
| src/c#/GeneralUpdate.Core/Configuration/ConfigurationMapper.cs | Throws on null source instead of silently mapping to an empty config. |
| src/c#/GeneralUpdate.Core/Compress/ZipCompressionStrategy.cs | Tightens zip entry matching during updates and adds path traversal guard during extraction. |
| src/c#/GeneralUpdate.Bowl/Strategies/WindowsBowlStrategy.cs | Avoids deleting existing fail directory to preserve diagnostics. |
| src/c#/GeneralUpdate.Bowl/Strategies/ProcessRunner.cs | Cancels delay timer when process exits first to reduce timer/resource leakage. |
| src/c#/GeneralUpdate.Bowl/Strategies/LinuxBowlStrategy.cs | Avoids deleting existing fail directory to preserve diagnostics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| static GeneralTracer() | ||
| { | ||
| Trace.Listeners.Clear(); |
Comment on lines
702
to
705
| // Compute absolute value safely: -long.MinValue would overflow, | ||
| // so clamp to long.MaxValue — the sign bit is stored separately. | ||
| long magnitude = value == long.MinValue ? long.MaxValue : (value < 0 ? -value : value); | ||
| buf[offset + 0] = (byte)(magnitude & 0xFF); |
Comment on lines
468
to
471
| // Compute absolute value safely: -long.MinValue would overflow, | ||
| // so clamp to long.MaxValue — the sign bit is stored separately. | ||
| long magnitude = value == long.MinValue ? long.MaxValue : (value < 0 ? -value : value); | ||
| buf[offset + 0] = (byte)(magnitude & 0xFF); |
Comment on lines
+156
to
+160
| var filePath = directoryInfo + entryFilePath; | ||
| var greatFolder = Directory.GetParent(filePath); | ||
| var fullTargetPath = Path.GetFullPath(filePath); | ||
| if (!fullTargetPath.StartsWith(extractionRoot, StringComparison.OrdinalIgnoreCase)) | ||
| throw new InvalidDataException( | ||
| $"Zip entry path traversal detected: {entries.FullName} resolves outside extraction directory."); |
Comment on lines
130
to
132
| @@ -131,11 +132,18 @@ private static bool IsRetryable(Exception ex) | |||
| if (ex is IOException) return true; | |||
Comment on lines
143
to
147
| var s = hre.Message ?? ""; | ||
| return s.Contains("timeout", StringComparison.OrdinalIgnoreCase) | ||
| || s.Contains("timed out", StringComparison.OrdinalIgnoreCase) | ||
| || s.Contains("500") || s.Contains("502") | ||
| || s.Contains("503") || s.Contains("504"); | ||
| || Regex.IsMatch(s, @"\b(500|502|503|504)\b"); | ||
| } |
Comment on lines
+525
to
+529
| var patchPrefix = patchPath.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; | ||
| var relativePart = file.FullName.StartsWith(patchPrefix, StringComparison.Ordinal) | ||
| ? file.FullName.Substring(patchPrefix.Length) | ||
| : file.FullName; | ||
| var targetPath = Path.Combine(appPath, relativePart); |
…ssExit deadlock, Trace listener leak, Strategy suicide, HubConnection lifecycle, RetryPolicy status code Comprehensive fix for ~25 bugs found in full code audit. BSDIFF/Differential: - Fix WriteInt64 long.MinValue overflow (BsdiffDiffer, StreamingHdiffDiffer) - Guard (int)control[0] truncation in Dirty method (BsdiffDiffer) - ReadFileWithBudget throws instead of silently truncating (StreamingHdiffDiffer) Zip security: - Add path-traversal guard in Decompress (ZipCompressionStrategy) - Fix StartsWith ambiguity — exact match instead of substring match Process lifecycle: - Fix ProcessExit deadlock (Task.Run wrapper in ClientStrategy.LaunchUpgradeProcessSync) - Move suicide from finally to success-only path (Windows/Linux/MacStrategy) - Track appLaunched flag so Bowl failure still triggers updater exit - GeneralTracer.Dispose removes only own listeners, not global Trace.Listeners Configuration & lifecycle: - Fix ProcessContract null-check ordering - Add disposed guard + null set to UpgradeHubService (HubConnection lifecycle) - ConfigurationMapper throws on null source instead of silent empty config - EventManager.Reset() for re-creatable singleton Network: - DefaultRetryPolicy uses HttpRequestException.StatusCode (.NET 5+) or regex for status matching - netstandard2.0 fallback uses \b(500|502|503|504)\b regex Co-Authored-By: Claude <noreply@anthropic.com>
JusterZhu
force-pushed
the
fix/code-review-bugs-202506
branch
5 times, most recently
from
a73a63d to
904ccf1
Compare
…n state across test classes
JusterZhu
force-pushed
the
fix/code-review-bugs-202506
branch
from
904ccf1 to
b846f75
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.
Summary
Fixes ~25 bugs from full code audit across GeneralUpdate.Core and GeneralUpdate.Differential.
🔴 Critical
WriteInt64long.MinValueoverflow (BsdiffDiffer + StreamingHdiffDiffer)(int)control[0]truncation in Dirty method — data loss on patches >2GBReadFileWithBudgetsilently truncates files >128MB../../evil.exe)StartsWithfuzzy match —foo.dllmatchesfoobar.dll🟠 High
GetAwaiter().GetResult()in sync handler (WPF/WinForms)finallykills updater on app launch failure, blocking recoveryTrace.Listeners.Clear()— GeneralTracer.Dispose nukes other libraries' trace outputProcessContract— null check order:Directory.Exists(null)before null guardUpgradeHubService—DisposeAsyncdoesn't null_connection→ NRE on reconnectConfigurationMapper— null source returns empty config silentlyDefaultRetryPolicy—s.Contains(\"500\")matches URL paths, not just status codes🟡 Medium
EventManager— addedReset()for re-creatable singletonCloses #514