Repository navigation
Add registry-validated DLL loading and Plugin Manager - #152
Conversation
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
…ture/dll-validation # Conflicts: # UnchainedLauncher.Core.Tests.Unit/Services/Dlls/DependencyTypesTests.cs
…ture/dll-validation
…ture/dll-validation
…ture/dll-validation
Jacoby6000
left a comment
Jacoby6000
left a comment
There was a problem hiding this comment.
Full independent review of all 16 PR commits and the complete base-to-head diff. Two reproduced issues below. The 60 focused Core tests, 27 portable GUI tests, and Windows-targeted GUI Release build pass. Live game injection remains untested.
| public void SetEnabled(DllRelease release, bool enabled) { | ||
| lock (_gate) { | ||
| EnsureCurrentSources(); | ||
| if (!Releases.Contains(release)) throw new InvalidOperationException("Unregistered DLL."); |
There was a problem hiding this comment.
[P2] Allow users to clear a removed plugin selection
After an enabled package is removed from the registry (or its registry is removed from settings), refresh leaves its pin in _enabled, but Plugin Manager builds cards only from Releases, so the package disappears. SetEnabled(oldRelease, false) also throws here. I reproduced zero displayed cards with {"Org/RemovedPlugin":"1.0.0"} still saved; both launch and update checks then fail with Enabled DLL is no longer registered. There is no UI recovery without editing the state file. Keep revoked DLLs blocked, but expose unavailable saved selections with a disable action and allow clearing a pin by identifier without requiring current registry approval. Cover refresh/restart after package or registry removal.
There was a problem hiding this comment.
Confirmed and fixed in 6d6e181. Plugin Manager now includes saved selections whose package or pinned version is no longer registered, labels them unavailable, and offers Disable. Clearing a pin by identifier requires no current registry approval; enabling and launch validation still fail closed for revoked releases. The existing selection-file format is unchanged, and disabling does not delete or approve loose DLLs. Deterministic production-manager/view-model tests cover authoritative package removal, registry removal, refresh/restart, a removed version with other releases remaining, and clearing selections while source approvals are stale. Validation: 62 portable Core tests and 36 portable GUI tests pass in Release; Windows-targeted GUI Release build passes (existing warnings). Live game/injection untested. Leaving this reviewer-owned thread unresolved for independent review.
| ImageUrls = release.Info.ImageUrls; | ||
| Authors = release.Info.Authors; | ||
| Dependencies = release.Info.Dependencies; | ||
| Dependencies = release.Info.EffectiveModDependencies; |
There was a problem hiding this comment.
[P2] Preserve plugin requirements when editing local releases
The form reads only EffectiveModDependencies, and ToRelease() reconstructs ModInfo with the old positional fields, leaving the new PluginDependencies null. Opening a local release with one plugin requirement, changing only its description, and saving therefore removes the requirement; the same happens when creating a new release from the existing release. I reproduced EffectivePluginDependencies.Count changing from 1 to 0, after which DependencyValidator.Validate([editedRelease], []) accepts the missing plugin. RegistryWindowService persists this reconstructed release through LocalModRegistry.AddRelease. Retain and round-trip the plugin dependency list (or reuse the original ModInfo record with a with update for edited fields) and add edit/new-release preservation tests; keep legacy pak dependency behavior unchanged.
There was a problem hiding this comment.
Confirmed and fixed in 6d6e181. RegistryReleaseFormVM retains the original ModInfo and uses a with update for editable fields, preserving PluginDependencies rather than reconstructing partial metadata. Effective pak requirements still round-trip through the existing legacy field without duplication. Moved the existing form class into its own production source file so portable tests compile the real form, not a reproduction. Tests cover editing and creating a new release from existing metadata, both legacy and modern pak dependencies, serialization round-trip, and rejection of the still-missing required plugin. Validation: 62 portable Core tests and 36 portable GUI tests pass in Release; Windows-targeted GUI Release build passes (existing warnings). Live game/injection untested. Leaving this reviewer-owned thread unresolved for independent review.
…ture/dll-validation
Jacoby6000
left a comment
Jacoby6000
left a comment
There was a problem hiding this comment.
Independent full review of base aa86426 through head ad3ec37: all PR commits and the complete diff, surrounding launch/injection, registry/configuration, dependency handling, UI, tests, and reuse/complexity. One new proven correctness finding below; prior findings 4253956669 and 4253956675 are fixed and are not duplicated. Portable Core tests pass 71/71 and GUI tests pass 36/36 in both Debug and Release; CRLF-aware diff whitespace check passes. Windows live injection/game launch and interactive UI could not be exercised on Linux. Verdict: address the archive deletion race before merging. No repository changes made.
| GetPath(candidate).Equals(path, StringComparison.OrdinalIgnoreCase) && MatchesFile(path, candidate)))) | ||
| continue; | ||
| CopyValidated(path, GetCachePath(release), release); | ||
| File.Delete(path); |
There was a problem hiding this comment.
[P2] Keep the validated source protected through archive deletion
CopyValidated closes its source read handle before returning, so this pathname deletion is no longer tied to the bytes that were approved. When disabling a plugin, switching versions, or migrating a legacy file, another process can write an unregistered version at path after the validated copy and before File.Delete; preparation then deletes that unknown file while the cache retains only the old registered bytes. _gate cannot exclude an external writer, and Windows FileShare.Read protection ends when the source stream closes. I reproduced this against the exact head using a disabled registered plugin with its cache removed: a writer waited for the cache to appear, wrote unknown user version to the active path, and preparation deleted it (Unknown active file preserved: False, cached content still registered binary); delaying entry to unlink made the existing interleaving deterministic. This violates the explicit requirement that unregistered versions remain untouched. Archive using a deletion operation bound to the same validated file identity while excluding writes/replacement through deletion (for example, an appropriate Windows handle-based operation), rather than closing the handle and deleting by path. Add a regression test for this interleaving; another hash check followed by a separate pathname delete would leave the same race.
There was a problem hiding this comment.
Confirmed and fixed in 9bcfd9a; final branch head 8483c16 includes the preserved formatter commit and Windows failure-path assertion correction. Archival now opens GENERIC_READ | DELETE with FILE_SHARE_READ, validates/copies/publishes while retaining that handle, then uses SetFileInformationByHandle(FileDispositionInfo) on the same validated identity. No separate pathname deletion or rehash/delete gap remains. Writes and replacement remain excluded until handle closure. Non-Windows archival explicitly fails closed; portable validation and ordinary copying remain available.
Deterministic production-helper regressions attempt a separate writer, pathname replacement, and deletion exactly after cache publication, verify each is blocked, confirm only approved bytes are archived, and confirm unknown bytes can subsequently be installed unchanged. Additional regression checks validation/publication failure never deletes the source. Existing preparation tests also verify portable fail-closed preservation.
Windows CI https://github.com/Chiv2-Community/UnchainedLauncher/actions/runs/38481349411 is green: 199 Core + 17 scanner tests in both Debug and Release, including all four new native regression cases. Linux portable Core harness: 75 tests passing in both configurations; Windows-targeted GUI build passes. Local/remote final head verified, clean tree. Thread left unresolved for independent review.
Jacoby6000
left a comment
Jacoby6000
left a comment
There was a problem hiding this comment.
Full independent review of all PR commits and the complete base-to-head diff. One new active-publication race below; the earlier archival-deletion race is fixed.
| input.CopyTo(output); | ||
| ValidateBytes(output, release); | ||
| } | ||
| File.Move(temp, destination, true); |
There was a problem hiding this comment.
[P2] Preserve files created during cache restoration
Active restoration calls this helper after !File.Exists(path) at line 382, but publication unconditionally overwrites the destination. Another process can create an unregistered DLL at that active path while the cache is being validated/copied; this move then destroys the unknown bytes without validating or archiving them. The source handle protects only the cache file, and _gate does not exclude external writers. I reproduced this twice against this head through PrepareAsync(allowDownloads: false): preload a valid 64 MiB cache, leave the active path absent, and use a filesystem watcher to write unknown user version at the active path when its staging .tmp appears. Preparation succeeds, Unknown active file preserved: False, and the active file contains the registered cache bytes. This is a destination-publication race, separate from the fixed archival-source deletion race. Publish active files with the existing non-overwriting File.Move overload and fail closed on a destination collision; keep cache replacement semantics separate. Add a regression that creates unknown bytes after the absence check and verifies launch fails without changing those bytes. Another existence/hash check followed by an overwrite would retain the race.
There was a problem hiding this comment.
Fixed in ab80b43; final remote head afb1747 includes only the subsequent formatter newline changes.
Independently reproduced with a deterministic production PrepareAsync boundary: after the absence check and validated staging copy, a separate writer creates unknown bytes at the active destination. Before the fix preparation succeeds; after the fix non-overwriting atomic File.Move throws IOException, preserves the unknown file and approved cache, removes staging, and injection rejects the unknown bytes. Cache publication retains replacement semantics; the prior Windows archival handle/same-handle deletion protection and Unix fail-closed behavior are unchanged.
Final-head Windows CI run 38483097711 passes all four jobs: 200 Core + 17 scanner tests in both Debug and Release, including the new regression. Local portable Core passes 76 tests in each configuration; GUI Release passes 36 tests using external source copies with only the temp-root adjusted for exhausted /tmp quota. Release Windows-targeted solution build passed with existing warnings. Live game injection remains untested. Thread left unresolved for fresh review.
Jacoby6000
left a comment
Jacoby6000
left a comment
There was a problem hiding this comment.
Independent review-only pass of base aa86426 through head 8ba0b24: one new verified P2 finding below. Reviewed all 34 PR commits including merge resolutions, the 55-file cumulative diff, surrounding runtime/configuration/storage contracts, upstream prepare-v1 schemas/live registry, and tests; assessed correctness, security, lifecycle/concurrency, regressions, reuse and complexity. Existing review fixes were inspected without duplicating their findings. DLL ownership/validation, authoritative package-list admission, cache-before-active preparation, validated injection handles, target filtering, dependency grouping, saved controls, prerelease startup and the single GUI -67 retry remain review constraints. Shared CLI/GUI pak inventory is explicitly deferred; separate server-cli untouched. Local portable Core/pak/DLL harness: 108 passing in Debug and Release; current GUI view-model/XML harness: 40 passing in Debug and Release (external test copies alter only temporary paths). Exact-head CI run 38583535009 succeeds in all four jobs; logs show 205 Core + 17 scanner tests passing per configuration on the base/head merge. CRLF-aware diff check passes. No additional actionable reuse/complexity finding; typed-object JSON round trips remain optional rather than demonstrated defects. Native game injection, interactive WPF, and live Steam retry were not exercised here. No repository edits, commits or pushes.
| var existing = existingByCoords.TryGetValue(item.Install.Coordinates, out var e) ? Some(e) : None; | ||
| return existing.Match( | ||
| // Keep a collision-safe installed name when the preferred name is user-owned. | ||
| Some: pak => pak.PakFileName == item.FinalPakName || unmanagedPaths.Contains(PakNameToPakPath(item.FinalPakName)) |
There was a problem hiding this comment.
[P2] Preserve the required load-order prefix when avoiding a collision
The new unmanaged-path condition skips relocation even when the existing filename has an obsolete sort prefix. Reproduced through production InstallModSet: put unmanaged q__-Dependency.pak and qz-Dependency.pak in the directory, install [Dependency] (creates managed q-Dependency (1).pak), then install [Dependency, Dependent]. Both calls succeed, but Dependency keeps q... while Dependent becomes qy__...; actual managed filename order is Dependency, Dependent rather than the required reversed dependency order Dependent, Dependency (qy..., qz...). Updating ManagedPak.Priority does not change the filename order used by the game, so adding a dependent can launch with incorrect pak precedence instead of failing closed. This is separate from the unchanged-single-pak retry collision covered by the new tests. Preserve an existing collision-safe filename only when it already has the required current sort prefix; otherwise reuse the textual-successor collision selection to move to an available name under the new prefix (including its .sig), preserve unmanaged bytes, and propagate move failures as preparation cancellation. Add a regression covering install-set growth/reordering with an occupied preferred destination.
There was a problem hiding this comment.
Fixed in 89aed4c. Collision-safe installed names are retained only when their current lexical load-order prefix matches the required prefix. Relocation now shares the existing textual-successor selection with downloads (case-insensitive collision checks), moves the .sig through the existing error-propagating path, and publishes inventory only after successful movement; unmanaged files are not overwritten.
Added production InstallModSet regressions for growth, reordered dependencies, shrink, multiple/case-insensitive collisions, persisted-inventory retries, fresh-install filename equivalence, pak/signature byte preservation, and pak/signature move errors; added a production launcher-boundary regression proving obsolete-prefix collision move failure cancels launch and closes progress. All five new cases fail with the old skip condition restored; fixed portable Core harness passes 113/113 in both Debug and Release. Scoped whitespace formatting and CRLF-aware diff checks pass. Native Windows CI run 38586208196 is queued. Leaving the thread unresolved for fresh independent review.
1 participant
Changes
Checks
CLI work remains in PR #150.