fix(core): resolve 16 audit findings — race conditions, design flaws, glue code, and performance issues

Closes #528

Author: JusterZhu

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

@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.IO;
 using System.Linq;
 using System.Threading;
@@ -158,17 +157,13 @@ private async Task<GeneralUpdateBootstrap> LaunchWithStrategy(IStrategy roleStra
             var sslPolicy = ResolveExtension<Security.ISslValidationPolicy>();
             if (sslPolicy != null)
             {
-                Network.VersionService.SetSslValidationPolicy(sslPolicy);
                 Network.HttpClientProvider.SetSslValidationPolicy(sslPolicy);
             }
             var authProvider = ResolveExtension<Security.IHttpAuthProvider>();
             if (authProvider != null) Network.VersionService.SetDefaultAuthProvider(authProvider);
 
             ConfigureStrategy(roleStrategy);
 
-            if (roleStrategy is ClientStrategy cs)
-                await CallSmallBowlHomeAsync(_configInfo.Bowl).ConfigureAwait(false);
-
             roleStrategy.Create(_configInfo);
 
             await roleStrategy.ExecuteAsync();
@@ -221,6 +216,9 @@ public GeneralUpdateBootstrap SetConfig(UpdateRequest configInfo)
         var appType = GetOption(Option.AppType);
         if (appType != AppType.Upgrade)
         {
+            // Cleanup temp directories from previous runs (older than 24h) to
+            // prevent disk accumulation from silent-mode polling or crashes.
+            StorageManager.CleanupOldTempDirectories();
             _configInfo.TempPath = StorageManager.GetTempDirectory("upgrade_temp");
             InitBlackPolicy();
         }
@@ -466,7 +464,6 @@ private async Task<GeneralUpdateBootstrap> LaunchSilentAsync()
         var sslPolicy = ResolveExtension<Security.ISslValidationPolicy>();
         if (sslPolicy != null)
         {
-            Network.VersionService.SetSslValidationPolicy(sslPolicy);
             Network.HttpClientProvider.SetSslValidationPolicy(sslPolicy);
         }
         var authProvider = ResolveExtension<Security.IHttpAuthProvider>();
@@ -577,38 +574,12 @@ private static Format ParseFormat(string? compressFormat)
 
     private void InitBlackPolicy()
     {
-        // Build blacklist matcher from UpdateContext and set on StorageManager.
-        // The matcher combines user config with system defaults.
-        var effectiveConfig = new BlackPolicy(
-            _configInfo.Files?.Count > 0 ? _configInfo.Files : BlackDefaults.DefaultFiles,
-            _configInfo.Formats?.Count > 0 ? _configInfo.Formats : BlackDefaults.DefaultFormats,
-            _configInfo.Directories?.Count > 0
-                ? _configInfo.Directories
-                : BlackDefaults.DefaultDirectories
-        );
-        StorageManager.BlackMatcher = new BlackMatcher(effectiveConfig);
-    }
-
-    private async Task CallSmallBowlHomeAsync(string processName)
-    {
-        if (string.IsNullOrWhiteSpace(processName))
-        {
-            GeneralTracer.Warn("CallSmallBowlHomeAsync: Bowl process name is empty or whitespace, skipping shutdown.");
-            return;
-        }
-        try
-        {
-            var processes = Process.GetProcessesByName(processName);
-            foreach (var process in processes)
-            {
-                GeneralTracer.Info($"Shutting down process {process.ProcessName} (ID: {process.Id})");
-                await GracefulExit.ShutdownAsync(process).ConfigureAwait(false);
-            }
-        }
-        catch (Exception ex)
-        {
-            GeneralTracer.Error("CallSmallBowlHomeAsync failed.", ex);
-        }
+        StorageManager.BlackMatcher = new BlackMatcher(
+            BlackDefaults.CreatePolicyWithDefaults(
+                _configInfo.Files,
+                _configInfo.Formats,
+                _configInfo.Directories
+            ));
     }
 
     // ════════════════════════════════════════════════════════════════

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

@@ -37,11 +37,6 @@ public class Option
         /// </summary>
         private static readonly ConcurrentDictionary<string, Option> _registry = new();
 
-        /// <summary>
-        ///     The synchronization lock object for thread-safe option creation.
-        /// </summary>
-        private static readonly object _lock = new();
-
         /// <summary>
         ///     The unique name identifier for this option.
         ///     Remains unique throughout the application lifetime.
@@ -74,15 +69,19 @@ protected Option(string name)
         /// <exception cref="ArgumentNullException">Thrown when <paramref name="name" /> is <c>null</c>.</exception>
         public static Option<T> ValueOf<T>(string name, T defaultValue = default!)
         {
-            lock (_lock)
-            {
-                if (_registry.TryGetValue(name, out var existing) && existing is Option<T> typed)
-                    return typed;
-
-                var option = new Option<T>(name, defaultValue);
-                _registry[name] = option;
-                return option;
-            }
+            // GetOrAdd is lock-free on ConcurrentDictionary. Under contention the
+            // factory may run multiple times, but only one instance is stored and
+            // returned by all racing callers. The factory must be side-effect-free.
+            var raw = _registry.GetOrAdd(name, _ => new Option<T>(name, defaultValue));
+
+            // If the existing entry was registered with a different type T, create a new one.
+            // This is the same "last writer wins" behavior as the original lock-based implementation.
+            if (raw is Option<T> typed)
+                return typed;
+
+            var replacement = new Option<T>(name, defaultValue);
+            _registry[name] = replacement;
+            return replacement;
         }
 
         /// <summary>

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

@@ -37,13 +37,7 @@ public class UpdateRequest : UpdateConfiguration
         ///         <item>
         ///             <c>UpdateLogUrl</c>: If set, must be a valid absolute URI.</item>
         ///         <item>
-        ///             <c>UpdateAppName</c>: Must not be empty.</item>
-        ///         <item>
-        ///             <c>MainAppName</c>: Must not be empty.</item>
-        ///         <item>
         ///             <c>AppSecretKey</c>: Must not be empty.</item>
-        ///         <item>
-        ///             <c>ClientVersion</c>: Must not be empty.</item>
         ///     </list>
         ///     <para>
         ///         This method is typically called at the end of the <see cref="UpdateRequestBuilder.Build" /> method

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

@@ -83,6 +83,20 @@ public static bool HasUpdate(
         return serverVersions.Max() > local;
     }
 
+    /// <summary>Pre-parses versions for a list of assets to avoid repeated Semver.TryParse calls.</summary>
+    private static Dictionary<DownloadAsset, SemVersion?> PreParseVersions(IEnumerable<DownloadAsset> assets)
+    {
+        var map = new Dictionary<DownloadAsset, SemVersion?>();
+        foreach (var a in assets)
+        {
+            // Custom IDownloadSource implementations could return duplicate records;
+            // silently accept the first occurrence rather than throwing.
+            if (!map.ContainsKey(a))
+                map[a] = ParseVersion(a.Version);
+        }
+        return map;
+    }
+
     /// <summary>
     /// Builds a download plan with AppType-aware version filtering.
     /// Client-type assets are compared against <paramref name="clientVersion"/>.
@@ -107,6 +121,16 @@ public static DownloadPlan Build(
         var parsedUpgrade = ParseVersion(upgradeClientVersion) ?? parsedClient;
         var uv = parsedUpgrade.Value;
 
+        // Pre-parse all asset versions to avoid repeated Semver.TryParse calls.
+        var versionMap = PreParseVersions(assets);
+
+        // Helper: safe lookup that matches netstandard2.0 (no GetValueOrDefault).
+        SemVersion? Lookup(DownloadAsset a)
+        {
+            versionMap.TryGetValue(a, out var sv);
+            return sv;
+        }
+
         // 1. Filter out frozen packages
         var active = assets
             .Where(a => !a.IsFreeze)
@@ -122,23 +146,22 @@ public static DownloadPlan Build(
         var candidates = active
             .Where(a =>
             {
-                if (!Semver.TryParse(a.Version, out var pv)) return false;
+                var pv = Lookup(a);
+                if (pv == null) return false;
 
                 var localVersion = (a.AppType == (int)AppType.Upgrade)
                     ? uv
                     : cv;
 
-                return pv > localVersion;
+                return pv.Value > localVersion;
             })
             .Where(a => IsCompatible(a.MinClientVersion, clientVersion))
-            .OrderBy(a => { Semver.TryParse(a.Version, out var sv); return sv; })
+            .OrderBy(a => Lookup(a))
             .ToList();
 
         if (candidates.Count == 0) return DownloadPlan.Empty;
 
-        // Separate chain vs full packages.
-        // Treat Unspecified (0) as Chain for backward compatibility with older
-        // servers that do not set PackageType yet.
+        // 4. Separate chain vs full packages.
         var chainCandidates = candidates
             .Where(a => a.PackageType == (int)Configuration.PackageType.Chain
                         || a.PackageType == (int)Configuration.PackageType.Unspecified)
@@ -149,62 +172,50 @@ public static DownloadPlan Build(
             .ToList();
 
         // ── Chain vs Full size-based decision ──
-        // If a full replacement package is available and the total chain download
-        // size approaches or exceeds the full package size, skip chain and use full.
         if (chainCandidates.Count > 0 && fullCandidates.Count > 0)
         {
-            // Pick the latest full package (highest version) across all AppTypes
             var bestFull = fullCandidates
-                .OrderByDescending(a => { Semver.TryParse(a.Version, out var sv); return sv; })
+                .OrderByDescending(a => Lookup(a))
                 .First();
 
-            // Only compare against chain packages of the same AppType as bestFull.
-            // Mixing Client and Upgrade sizes together could trigger incorrect switching.
             long chainTotal = chainCandidates
                 .Where(a => a.AppType == bestFull.AppType)
                 .Sum(a => a.Size);
             var threshold = (long)(bestFull.Size * 0.8);
 
             if (chainTotal >= threshold)
             {
-                // Chain is too expensive — use full package instead.
-                // Supplement with chain packages for other AppTypes not covered by full.
                 GeneralTracer.Info($"DownloadPlanBuilder: chain total {chainTotal} >= 80% of full size {bestFull.Size}, switching to full package {bestFull.Name}");
+                var bestFullSv = Lookup(bestFull);
                 var planAssets = new List<DownloadAsset> { bestFull };
                 planAssets.AddRange(chainCandidates
                     .Where(a => a.AppType != bestFull.AppType
-                                || (Semver.TryParse(a.Version, out var av)
-                                    && Semver.TryParse(bestFull.Version, out var fv)
-                                    && av > fv))
-                    .OrderBy(a => { Semver.TryParse(a.Version, out var sv); return sv; }));
+                                || (Lookup(a) is { } av && bestFullSv != null && av > bestFullSv))
+                    .OrderBy(a => Lookup(a)));
                 return new DownloadPlan(planAssets, isForcibly);
             }
         }
 
         // ── Chain plan with fallback fulls ──
-        // Use chain packages normally. Attach FallbackFull* info to each chain entry
-        // so that if a chain patch fails, AbstractStrategy can fall back to full.
         if (fullCandidates.Count > 0)
         {
             var fallbackFulls = new List<DownloadAsset>();
 
             var chainWithFallback = chainCandidates
                 .Select(chain =>
                 {
-                    // Find a matching full: same AppType + same Version (or closest)
                     var match = fullCandidates
                         .Where(f => f.AppType == chain.AppType)
-                        .OrderBy(f => { Semver.TryParse(f.Version, out var sv); return sv; })
+                        .OrderBy(f => Lookup(f))
                         .FirstOrDefault(f =>
                         {
-                            if (!Semver.TryParse(f.Version, out var fv)) return false;
-                            if (!Semver.TryParse(chain.Version, out var cv)) return false;
-                            return fv >= cv;
+                            var fv = Lookup(f);
+                            var cv = Lookup(chain);
+                            return fv != null && cv != null && fv.Value >= cv.Value;
                         });
 
                     if (match != null)
                     {
-                        // Add matching full to the fallback list once
                         if (!fallbackFulls.Any(f => f.Url == match.Url))
                             fallbackFulls.Add(match);
 
@@ -226,7 +237,6 @@ public static DownloadPlan Build(
             };
         }
 
-        // No full packages at all: return chain packages as-is
         return new DownloadPlan(chainCandidates, isForcibly);
     }
 
@@ -245,11 +255,7 @@ public static DownloadPlan Build(IEnumerable<DownloadAsset> assets, string curre
 
     /// <summary>
     /// Checks whether the specified MinClientVersion is compatible with the current client version.
-    /// If a package's MinClientVersion is higher than the current version, the package is not applicable.
     /// </summary>
-    /// <param name="minClientVersion">The minimum client version required by the package. If null or empty, the package is considered compatible.</param>
-    /// <param name="currentVersion">The current client version string.</param>
-    /// <returns>True if the current version meets or exceeds the minimum requirement; otherwise false.</returns>
     internal static bool IsCompatible(string? minClientVersion, string currentVersion)
     {
         if (string.IsNullOrEmpty(minClientVersion)) return true;
@@ -260,8 +266,6 @@ internal static bool IsCompatible(string? minClientVersion, string currentVersio
     }
 
     /// <summary>Parses a version string and returns null if the string cannot be parsed.</summary>
-    /// <param name="version">The version string to parse.</param>
-    /// <returns>A parsed <see cref="SemVersion"/> value, or null if parsing fails.</returns>
     internal static SemVersion? ParseVersion(string? version)
     {
         if (string.IsNullOrWhiteSpace(version)) return null;

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

@@ -210,10 +210,10 @@ public async Task<DownloadReport> ExecuteAsync(
 
         var tasks = plan.Assets.Select(async asset =>
         {
-            var acquired = await sem.WaitAsync(TimeSpan.FromMinutes(5), token).ConfigureAwait(false);
+            var acquired = await sem.WaitAsync(TimeSpan.FromMinutes(1), token).ConfigureAwait(false);
             if (!acquired)
             {
-                GeneralTracer.Warn("DefaultDownloadOrchestrator: semaphore wait timed out for " + asset.Name + ", skipping.");
+                GeneralTracer.Warn($"DefaultDownloadOrchestrator: semaphore wait timed out (1 min) for '{asset.Name}'. Concurrency={effectiveConcurrency}, ActiveSlots={effectiveConcurrency - sem.CurrentCount}. Skipping asset.");
                 lock (results)
                 {
                     results.Add(new DownloadResult(asset, null, 0, TimeSpan.Zero, 0, false, "Semaphore wait timed out"));

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

@@ -28,4 +28,21 @@ public static class BlackDefaults
         StorageManager.LegacyDirectoryPrefix,
         "fail"
     };
+
+    /// <summary>
+    /// Creates a <see cref="BlackPolicy"/> using caller-provided lists, falling back to
+    /// <see cref="DefaultFiles"/>, <see cref="DefaultFormats"/>, and <see cref="DefaultDirectories"/>
+    /// for any list that is null or empty.
+    /// </summary>
+    public static BlackPolicy CreatePolicyWithDefaults(
+        IReadOnlyList<string>? files,
+        IReadOnlyList<string>? formats,
+        IReadOnlyList<string>? directories)
+    {
+        return new BlackPolicy(
+            files?.Count > 0 ? new List<string>(files) : DefaultFiles,
+            formats?.Count > 0 ? new List<string>(formats) : DefaultFormats,
+            directories?.Count > 0 ? new List<string>(directories) : DefaultDirectories
+        );
+    }
 }

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

@@ -95,9 +95,16 @@ public bool IsBlacklistedFormat(string extension)
     /// Uses case-insensitive substring containment matching (<c>string.IndexOf</c>) for evaluation.
     /// The directory is considered skippable if its name contains any string from the <c>Directories</c> list.
     /// </remarks>
-    public bool ShouldSkipDirectory(string directoryName)
-        => _config.Directories?.Any(d =>
-            directoryName.IndexOf(d, StringComparison.OrdinalIgnoreCase) >= 0) == true;
+    public bool ShouldSkipDirectory(string directoryOrPath)
+    {
+        // Extract the directory name from a possible full path so both
+        // bare names ("backup-2026") and full paths (".backups/backup-2026") work.
+        var dirName = Path.GetFileName(directoryOrPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar));
+        if (string.IsNullOrEmpty(dirName)) return false;
+
+        return _config.Directories?.Any(d =>
+            dirName.StartsWith(d, StringComparison.OrdinalIgnoreCase)) == true;
+    }
 
     /// <summary>
     /// Matches a file name against a simple Glob pattern.