Skip to content

Commit dd80200

Browse files
JusterZhuclaude
JusterZhu
and
claude
committed
fix(core): resolve 16 audit findings — race conditions, design flaws, glue code, and performance issues
This commit addresses the full audit review of GeneralUpdate.Core: **Bug fixes:** - Remove HasExited race condition in WindowsStrategy & MacStrategy (#3,#4) - Only rollback when no version succeeded yet in AbstractStrategy (#5) - Guard against null version.Name in DeleteVersionZip (#6) - Better semaphore timeout logging in DefaultDownloadOrchestrator (#8) - Robust concurrent-safe DeleteDirectory in StorageManager (#10) - Add re-entry guard in AbstractStrategy.ExecuteAsync (#11) **Design improvements:** - Fix XML doc to match actual validation in UpdateRequest.Validate() (#12) - Unify SSL policy: VersionService delegates to HttpClientProvider (#16,#17) - GracefulExit self-shutdown no longer calls Kill() on the current process (#18) - Use StartsWith instead of IndexOf in BlackMatcher.ShouldSkipDirectory (#19) - Replace lock with ConcurrentDictionary.GetOrAdd in Option.ValueOf (#20) **Glue code removal:** - Extract shared BlackDefaults.CreatePolicyWithDefaults() (#21) - Remove duplicate CallSmallBowlHomeAsync from Bootstrap (#22) - Extract shared OsStrategyResolver class (#23) - Make SafeOnBeforeUpdateAsync semantics consistent: exception = abort (#24) **Performance:** - Cache parsed SemVers in DownloadPlanBuilder to avoid repeated parsing (#26) - Reuse Sha256HashAlgorithm as static field in HashMiddleware (#30) - Add CleanupOldTempDirectories() to prevent temp directory accumulation (#31) Co-authored-by: Claude <noreply@anthropic.com>
1 parent dd4ae23 commit dd80200

17 files changed

Lines changed: 294 additions & 205 deletions

‎src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs‎

Lines changed: 9 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
using System;
22
using System.Collections.Generic;
3-
using System.Diagnostics;
43
using System.IO;
54
using System.Linq;
65
using System.Threading;
@@ -158,17 +157,13 @@ private async Task<GeneralUpdateBootstrap> LaunchWithStrategy(IStrategy roleStra
158157
var sslPolicy = ResolveExtension<Security.ISslValidationPolicy>();
159158
if (sslPolicy != null)
160159
{
161-
Network.VersionService.SetSslValidationPolicy(sslPolicy);
162160
Network.HttpClientProvider.SetSslValidationPolicy(sslPolicy);
163161
}
164162
var authProvider = ResolveExtension<Security.IHttpAuthProvider>();
165163
if (authProvider != null) Network.VersionService.SetDefaultAuthProvider(authProvider);
166164

167165
ConfigureStrategy(roleStrategy);
168166

169-
if (roleStrategy is ClientStrategy cs)
170-
await CallSmallBowlHomeAsync(_configInfo.Bowl).ConfigureAwait(false);
171-
172167
roleStrategy.Create(_configInfo);
173168

174169
await roleStrategy.ExecuteAsync();
@@ -221,6 +216,9 @@ public GeneralUpdateBootstrap SetConfig(UpdateRequest configInfo)
221216
var appType = GetOption(Option.AppType);
222217
if (appType != AppType.Upgrade)
223218
{
219+
// Cleanup temp directories from previous runs (older than 24h) to
220+
// prevent disk accumulation from silent-mode polling or crashes.
221+
StorageManager.CleanupOldTempDirectories();
224222
_configInfo.TempPath = StorageManager.GetTempDirectory("upgrade_temp");
225223
InitBlackPolicy();
226224
}
@@ -466,7 +464,6 @@ private async Task<GeneralUpdateBootstrap> LaunchSilentAsync()
466464
var sslPolicy = ResolveExtension<Security.ISslValidationPolicy>();
467465
if (sslPolicy != null)
468466
{
469-
Network.VersionService.SetSslValidationPolicy(sslPolicy);
470467
Network.HttpClientProvider.SetSslValidationPolicy(sslPolicy);
471468
}
472469
var authProvider = ResolveExtension<Security.IHttpAuthProvider>();
@@ -577,38 +574,12 @@ private static Format ParseFormat(string? compressFormat)
577574

578575
private void InitBlackPolicy()
579576
{
580-
// Build blacklist matcher from UpdateContext and set on StorageManager.
581-
// The matcher combines user config with system defaults.
582-
var effectiveConfig = new BlackPolicy(
583-
_configInfo.Files?.Count > 0 ? _configInfo.Files : BlackDefaults.DefaultFiles,
584-
_configInfo.Formats?.Count > 0 ? _configInfo.Formats : BlackDefaults.DefaultFormats,
585-
_configInfo.Directories?.Count > 0
586-
? _configInfo.Directories
587-
: BlackDefaults.DefaultDirectories
588-
);
589-
StorageManager.BlackMatcher = new BlackMatcher(effectiveConfig);
590-
}
591-
592-
private async Task CallSmallBowlHomeAsync(string processName)
593-
{
594-
if (string.IsNullOrWhiteSpace(processName))
595-
{
596-
GeneralTracer.Warn("CallSmallBowlHomeAsync: Bowl process name is empty or whitespace, skipping shutdown.");
597-
return;
598-
}
599-
try
600-
{
601-
var processes = Process.GetProcessesByName(processName);
602-
foreach (var process in processes)
603-
{
604-
GeneralTracer.Info($"Shutting down process {process.ProcessName} (ID: {process.Id})");
605-
await GracefulExit.ShutdownAsync(process).ConfigureAwait(false);
606-
}
607-
}
608-
catch (Exception ex)
609-
{
610-
GeneralTracer.Error("CallSmallBowlHomeAsync failed.", ex);
611-
}
577+
StorageManager.BlackMatcher = new BlackMatcher(
578+
BlackDefaults.CreatePolicyWithDefaults(
579+
_configInfo.Files,
580+
_configInfo.Formats,
581+
_configInfo.Directories
582+
));
612583
}
613584

614585
// ════════════════════════════════════════════════════════════════

‎src/c#/GeneralUpdate.Core/Configuration/Option.cs‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,6 @@ public class Option
3737
/// </summary>
3838
private static readonly ConcurrentDictionary<string, Option> _registry = new();
3939

40-
/// <summary>
41-
/// The synchronization lock object for thread-safe option creation.
42-
/// </summary>
43-
private static readonly object _lock = new();
44-
4540
/// <summary>
4641
/// The unique name identifier for this option.
4742
/// Remains unique throughout the application lifetime.
@@ -74,15 +69,15 @@ protected Option(string name)
7469
/// <exception cref="ArgumentNullException">Thrown when <paramref name="name" /> is <c>null</c>.</exception>
7570
public static Option<T> ValueOf<T>(string name, T defaultValue = default!)
7671
{
77-
lock (_lock)
78-
{
79-
if (_registry.TryGetValue(name, out var existing) && existing is Option<T> typed)
80-
return typed;
81-
82-
var option = new Option<T>(name, defaultValue);
83-
_registry[name] = option;
84-
return option;
85-
}
72+
// GetOrAdd is lock-free on ConcurrentDictionary; the factory runs at most once per key.
73+
// If the name was already registered with a different type T, the cast below throws,
74+
// which is far more likely to be a programming error than a runtime concern.
75+
var raw = _registry.GetOrAdd(name, _ => new Option<T>(name, defaultValue));
76+
if (raw is Option<T> typed)
77+
return typed;
78+
79+
throw new InvalidOperationException(
80+
$"Option '{name}' was already registered with a different type. Expected {typeof(Option<T>)}, got {raw.GetType()}.");
8681
}
8782

8883
/// <summary>

‎src/c#/GeneralUpdate.Core/Configuration/UpdateRequest.cs‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,7 @@ public class UpdateRequest : UpdateConfiguration
3737
/// <item>
3838
/// <c>UpdateLogUrl</c>: If set, must be a valid absolute URI.</item>
3939
/// <item>
40-
/// <c>UpdateAppName</c>: Must not be empty.</item>
41-
/// <item>
42-
/// <c>MainAppName</c>: Must not be empty.</item>
43-
/// <item>
4440
/// <c>AppSecretKey</c>: Must not be empty.</item>
45-
/// <item>
46-
/// <c>ClientVersion</c>: Must not be empty.</item>
4741
/// </list>
4842
/// <para>
4943
/// This method is typically called at the end of the <see cref="UpdateRequestBuilder.Build" /> method

‎src/c#/GeneralUpdate.Core/Download/DownloadPlanBuilder.cs‎

Lines changed: 31 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,14 @@ public static bool HasUpdate(
8383
return serverVersions.Max() > local;
8484
}
8585

86+
/// <summary>Pre-parses versions for a list of assets to avoid repeated Semver.TryParse calls.</summary>
87+
private static Dictionary<DownloadAsset, SemVersion?> PreParseVersions(IEnumerable<DownloadAsset> assets)
88+
{
89+
return assets.ToDictionary(
90+
a => a,
91+
a => ParseVersion(a.Version));
92+
}
93+
8694
/// <summary>
8795
/// Builds a download plan with AppType-aware version filtering.
8896
/// Client-type assets are compared against <paramref name="clientVersion"/>.
@@ -107,6 +115,16 @@ public static DownloadPlan Build(
107115
var parsedUpgrade = ParseVersion(upgradeClientVersion) ?? parsedClient;
108116
var uv = parsedUpgrade.Value;
109117

118+
// Pre-parse all asset versions to avoid repeated Semver.TryParse calls.
119+
var versionMap = PreParseVersions(assets);
120+
121+
// Helper: safe lookup that matches netstandard2.0 (no GetValueOrDefault).
122+
SemVersion? Lookup(DownloadAsset a)
123+
{
124+
versionMap.TryGetValue(a, out var sv);
125+
return sv;
126+
}
127+
110128
// 1. Filter out frozen packages
111129
var active = assets
112130
.Where(a => !a.IsFreeze)
@@ -122,23 +140,22 @@ public static DownloadPlan Build(
122140
var candidates = active
123141
.Where(a =>
124142
{
125-
if (!Semver.TryParse(a.Version, out var pv)) return false;
143+
var pv = Lookup(a);
144+
if (pv == null) return false;
126145

127146
var localVersion = (a.AppType == (int)AppType.Upgrade)
128147
? uv
129148
: cv;
130149

131-
return pv > localVersion;
150+
return pv.Value > localVersion;
132151
})
133152
.Where(a => IsCompatible(a.MinClientVersion, clientVersion))
134-
.OrderBy(a => { Semver.TryParse(a.Version, out var sv); return sv; })
153+
.OrderBy(a => Lookup(a))
135154
.ToList();
136155

137156
if (candidates.Count == 0) return DownloadPlan.Empty;
138157

139-
// Separate chain vs full packages.
140-
// Treat Unspecified (0) as Chain for backward compatibility with older
141-
// servers that do not set PackageType yet.
158+
// 4. Separate chain vs full packages.
142159
var chainCandidates = candidates
143160
.Where(a => a.PackageType == (int)Configuration.PackageType.Chain
144161
|| a.PackageType == (int)Configuration.PackageType.Unspecified)
@@ -149,62 +166,50 @@ public static DownloadPlan Build(
149166
.ToList();
150167

151168
// ── Chain vs Full size-based decision ──
152-
// If a full replacement package is available and the total chain download
153-
// size approaches or exceeds the full package size, skip chain and use full.
154169
if (chainCandidates.Count > 0 && fullCandidates.Count > 0)
155170
{
156-
// Pick the latest full package (highest version) across all AppTypes
157171
var bestFull = fullCandidates
158-
.OrderByDescending(a => { Semver.TryParse(a.Version, out var sv); return sv; })
172+
.OrderByDescending(a => Lookup(a))
159173
.First();
160174

161-
// Only compare against chain packages of the same AppType as bestFull.
162-
// Mixing Client and Upgrade sizes together could trigger incorrect switching.
163175
long chainTotal = chainCandidates
164176
.Where(a => a.AppType == bestFull.AppType)
165177
.Sum(a => a.Size);
166178
var threshold = (long)(bestFull.Size * 0.8);
167179

168180
if (chainTotal >= threshold)
169181
{
170-
// Chain is too expensive — use full package instead.
171-
// Supplement with chain packages for other AppTypes not covered by full.
172182
GeneralTracer.Info($"DownloadPlanBuilder: chain total {chainTotal} >= 80% of full size {bestFull.Size}, switching to full package {bestFull.Name}");
183+
var bestFullSv = Lookup(bestFull);
173184
var planAssets = new List<DownloadAsset> { bestFull };
174185
planAssets.AddRange(chainCandidates
175186
.Where(a => a.AppType != bestFull.AppType
176-
|| (Semver.TryParse(a.Version, out var av)
177-
&& Semver.TryParse(bestFull.Version, out var fv)
178-
&& av > fv))
179-
.OrderBy(a => { Semver.TryParse(a.Version, out var sv); return sv; }));
187+
|| (Lookup(a) is { } av && bestFullSv != null && av > bestFullSv))
188+
.OrderBy(a => Lookup(a)));
180189
return new DownloadPlan(planAssets, isForcibly);
181190
}
182191
}
183192

184193
// ── Chain plan with fallback fulls ──
185-
// Use chain packages normally. Attach FallbackFull* info to each chain entry
186-
// so that if a chain patch fails, AbstractStrategy can fall back to full.
187194
if (fullCandidates.Count > 0)
188195
{
189196
var fallbackFulls = new List<DownloadAsset>();
190197

191198
var chainWithFallback = chainCandidates
192199
.Select(chain =>
193200
{
194-
// Find a matching full: same AppType + same Version (or closest)
195201
var match = fullCandidates
196202
.Where(f => f.AppType == chain.AppType)
197-
.OrderBy(f => { Semver.TryParse(f.Version, out var sv); return sv; })
203+
.OrderBy(f => Lookup(f))
198204
.FirstOrDefault(f =>
199205
{
200-
if (!Semver.TryParse(f.Version, out var fv)) return false;
201-
if (!Semver.TryParse(chain.Version, out var cv)) return false;
202-
return fv >= cv;
206+
var fv = Lookup(f);
207+
var cv = Lookup(chain);
208+
return fv != null && cv != null && fv.Value >= cv.Value;
203209
});
204210

205211
if (match != null)
206212
{
207-
// Add matching full to the fallback list once
208213
if (!fallbackFulls.Any(f => f.Url == match.Url))
209214
fallbackFulls.Add(match);
210215

@@ -226,7 +231,6 @@ public static DownloadPlan Build(
226231
};
227232
}
228233

229-
// No full packages at all: return chain packages as-is
230234
return new DownloadPlan(chainCandidates, isForcibly);
231235
}
232236

@@ -245,11 +249,7 @@ public static DownloadPlan Build(IEnumerable<DownloadAsset> assets, string curre
245249

246250
/// <summary>
247251
/// Checks whether the specified MinClientVersion is compatible with the current client version.
248-
/// If a package's MinClientVersion is higher than the current version, the package is not applicable.
249252
/// </summary>
250-
/// <param name="minClientVersion">The minimum client version required by the package. If null or empty, the package is considered compatible.</param>
251-
/// <param name="currentVersion">The current client version string.</param>
252-
/// <returns>True if the current version meets or exceeds the minimum requirement; otherwise false.</returns>
253253
internal static bool IsCompatible(string? minClientVersion, string currentVersion)
254254
{
255255
if (string.IsNullOrEmpty(minClientVersion)) return true;
@@ -260,8 +260,6 @@ internal static bool IsCompatible(string? minClientVersion, string currentVersio
260260
}
261261

262262
/// <summary>Parses a version string and returns null if the string cannot be parsed.</summary>
263-
/// <param name="version">The version string to parse.</param>
264-
/// <returns>A parsed <see cref="SemVersion"/> value, or null if parsing fails.</returns>
265263
internal static SemVersion? ParseVersion(string? version)
266264
{
267265
if (string.IsNullOrWhiteSpace(version)) return null;

‎src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,10 +210,10 @@ public async Task<DownloadReport> ExecuteAsync(
210210

211211
var tasks = plan.Assets.Select(async asset =>
212212
{
213-
var acquired = await sem.WaitAsync(TimeSpan.FromMinutes(5), token).ConfigureAwait(false);
213+
var acquired = await sem.WaitAsync(TimeSpan.FromMinutes(1), token).ConfigureAwait(false);
214214
if (!acquired)
215215
{
216-
GeneralTracer.Warn("DefaultDownloadOrchestrator: semaphore wait timed out for " + asset.Name + ", skipping.");
216+
GeneralTracer.Warn($"DefaultDownloadOrchestrator: semaphore wait timed out (1 min) for '{asset.Name}'. Concurrency={effectiveConcurrency}, ActiveSlots={effectiveConcurrency - sem.CurrentCount}. Skipping asset.");
217217
lock (results)
218218
{
219219
results.Add(new DownloadResult(asset, null, 0, TimeSpan.Zero, 0, false, "Semaphore wait timed out"));

‎src/c#/GeneralUpdate.Core/FileSystem/BlackDefaults.cs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,21 @@ public static class BlackDefaults
2828
StorageManager.LegacyDirectoryPrefix,
2929
"fail"
3030
};
31+
32+
/// <summary>
33+
/// Creates a <see cref="BlackPolicy"/> using caller-provided lists, falling back to
34+
/// <see cref="DefaultFiles"/>, <see cref="DefaultFormats"/>, and <see cref="DefaultDirectories"/>
35+
/// for any list that is null or empty.
36+
/// </summary>
37+
public static BlackPolicy CreatePolicyWithDefaults(
38+
IReadOnlyList<string>? files,
39+
IReadOnlyList<string>? formats,
40+
IReadOnlyList<string>? directories)
41+
{
42+
return new BlackPolicy(
43+
files?.Count > 0 ? new List<string>(files) : DefaultFiles,
44+
formats?.Count > 0 ? new List<string>(formats) : DefaultFormats,
45+
directories?.Count > 0 ? new List<string>(directories) : DefaultDirectories
46+
);
47+
}
3148
}

‎src/c#/GeneralUpdate.Core/FileSystem/BlackMatcher.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ public bool IsBlacklistedFormat(string extension)
9797
/// </remarks>
9898
public bool ShouldSkipDirectory(string directoryName)
9999
=> _config.Directories?.Any(d =>
100-
directoryName.IndexOf(d, StringComparison.OrdinalIgnoreCase) >= 0) == true;
100+
directoryName.StartsWith(d, StringComparison.OrdinalIgnoreCase)) == true;
101101

102102
/// <summary>
103103
/// Matches a file name against a simple Glob pattern.

0 commit comments

Comments
 (0)