Skip to content

Commit a2d3927

Browse files
JusterZhuclaude
andcommitted
fix: address 6 Copilot review suggestions — long.MinValue throw, Trace.Listeners.Clear, path traversal hardening, retry catch order, regex tightening, StartsWith case
- BsdiffDiffer/StreamingHdiffDiffer WriteInt64: throw on long.MinValue instead of silent clamp - GeneralTracer: remove Trace.Listeners.Clear() from static ctor; only manage owned listeners - ZipCompressionStrategy: use Path.Combine + normalized root in path-traversal guard - DefaultRetryPolicy: fix TaskCanceledException/OperationCanceledException catch order; tighten 5xx regex - DiffPipeline CopyUnknownFiles: use platform-aware path comparison (OrdinalIgnoreCase on Windows) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 3c35b1d commit a2d3927

6 files changed

Lines changed: 25 additions & 12 deletions

File tree

src/c#/GeneralUpdate.Core/Compress/ZipCompressionStrategy.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,12 @@ public void Decompress(string zipFilePath, string unZipDir, Encoding encoding)
153153
}
154154

155155
// Guard against path-traversal entries (e.g. "../../evil.exe")
156-
var filePath = directoryInfo + entryFilePath;
156+
var filePath = Path.Combine(unZipDir, entryFilePath);
157157
var fullTargetPath = Path.GetFullPath(filePath);
158-
if (!fullTargetPath.StartsWith(extractionRoot, StringComparison.OrdinalIgnoreCase))
158+
var rootWithSep = extractionRoot.EndsWith(dirSeparatorChar)
159+
? extractionRoot
160+
: extractionRoot + dirSeparatorChar;
161+
if (!fullTargetPath.StartsWith(rootWithSep, StringComparison.OrdinalIgnoreCase))
159162
throw new InvalidDataException(
160163
$"Zip entry path traversal detected: {entries.FullName} resolves outside extraction directory.");
161164

src/c#/GeneralUpdate.Core/Download/Policy/DefaultRetryPolicy.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,10 @@ public async Task<T> ExecuteAsync<T>(Func<CancellationToken, Task<T>> action, Ca
127127
/// </remarks>
128128
private static bool IsRetryable(Exception ex)
129129
{
130-
if (ex is OperationCanceledException) return false;
130+
// TaskCanceledException derives from OperationCanceledException,
131+
// so check it first — timeouts should be retryable.
131132
if (ex is TaskCanceledException or TimeoutException) return true;
133+
if (ex is OperationCanceledException) return false;
132134
if (ex is IOException) return true;
133135
if (ex is HttpRequestException hre)
134136
{
@@ -143,7 +145,7 @@ private static bool IsRetryable(Exception ex)
143145
var s = hre.Message ?? "";
144146
return s.Contains("timeout", StringComparison.OrdinalIgnoreCase)
145147
|| s.Contains("timed out", StringComparison.OrdinalIgnoreCase)
146-
|| Regex.IsMatch(s, @"\b(500|502|503|504)\b");
148+
|| Regex.IsMatch(s, @"\b(500|502|503|504)\b(?![\d./])");
147149
}
148150
return false;
149151
}

src/c#/GeneralUpdate.Core/Pipeline/DiffPipeline.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
using System.Collections.Generic;
33
using System.IO;
44
using System.Linq;
5+
using System.Runtime.InteropServices;
56
using System.Threading;
67
using System.Threading.Tasks;
78
using GeneralUpdate.Core.Differential;
@@ -523,7 +524,10 @@ private static Task CopyUnknownFiles(string appPath, string patchPath)
523524
// Using StartsWith + Substring instead of string.Replace to avoid
524525
// incorrect replacements when appPath is a substring of patchPath.
525526
var patchPrefix = patchPath.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar;
526-
var relativePart = file.FullName.StartsWith(patchPrefix, StringComparison.Ordinal)
527+
var comparison = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
528+
? StringComparison.OrdinalIgnoreCase
529+
: StringComparison.Ordinal;
530+
var relativePart = file.FullName.StartsWith(patchPrefix, comparison)
527531
? file.FullName.Substring(patchPrefix.Length)
528532
: file.FullName;
529533
var targetPath = Path.Combine(appPath, relativePart);

src/c#/GeneralUpdate.Core/Tracer/GeneralTracer.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@ public static class GeneralTracer
1717

1818
static GeneralTracer()
1919
{
20-
Trace.Listeners.Clear();
20+
// Do NOT call Trace.Listeners.Clear() here — other libraries in the
21+
// process may have registered their own TraceListeners. GeneralTracer
22+
// only manages the listeners it adds itself via _ownedListeners.
2123

2224
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
2325
{

src/c#/GeneralUpdate.Differential/Differ/BsdiffDiffer.cs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -696,12 +696,14 @@ private static int[] SuffixSort(byte[] oldData)
696696
/// Writes a 64-bit signed integer in BSDIFF sign-magnitude encoding.
697697
/// Bytes 0-6: magnitude bits 0-55. Byte 7 lower 7 bits: magnitude bits 56-62.
698698
/// Byte 7 upper bit: sign flag (1 = negative).
699+
/// Throws <see cref="ArgumentOutOfRangeException"/> when <paramref name="value"/>
700+
/// is <see cref="long.MinValue"/> because BSDIFF sign-magnitude encoding cannot
701+
/// represent -2^63 (requires a 64th magnitude bit).
699702
/// </summary>
700703
private static void WriteInt64(long value, byte[] buf, int offset)
701704
{
702-
// Compute absolute value safely: -long.MinValue would overflow,
703-
// so clamp to long.MaxValue — the sign bit is stored separately.
704-
long magnitude = value == long.MinValue ? long.MaxValue : (value < 0 ? -value : value);
705+
ArgumentOutOfRangeException.ThrowIfEqual(value, long.MinValue, nameof(value));
706+
long magnitude = value < 0 ? -value : value;
705707
buf[offset + 0] = (byte)(magnitude & 0xFF);
706708
buf[offset + 1] = (byte)((magnitude >> 8) & 0xFF);
707709
buf[offset + 2] = (byte)((magnitude >> 16) & 0xFF);

src/c#/GeneralUpdate.Differential/Differ/StreamingHdiffDiffer.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -465,9 +465,9 @@ private static byte[] ReadFileWithBudget(string path, int maxBytes)
465465

466466
private static void WriteInt64(long value, byte[] buf, int offset)
467467
{
468-
// Compute absolute value safely: -long.MinValue would overflow,
469-
// so clamp to long.MaxValue — the sign bit is stored separately.
470-
long magnitude = value == long.MinValue ? long.MaxValue : (value < 0 ? -value : value);
468+
// BSDIFF sign-magnitude encoding cannot represent -2^63.
469+
ArgumentOutOfRangeException.ThrowIfEqual(value, long.MinValue, nameof(value));
470+
long magnitude = value < 0 ? -value : value;
471471
buf[offset + 0] = (byte)(magnitude & 0xFF);
472472
buf[offset + 1] = (byte)((magnitude >> 8) & 0xFF);
473473
buf[offset + 2] = (byte)((magnitude >> 16) & 0xFF);

0 commit comments

Comments
 (0)