ncrypt persisted keys, and reaping launchers whose wineserver is gone - #16
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5266ede711
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| + SECURITY_STATUS ret = write_key_store(object->key.name, object->key.algid, object->key.bcrypt_key); | ||
| + if (ret != ERROR_SUCCESS) | ||
| + WARN("could not store key %s, it will not survive this process\n", wine_dbgstr_w(object->key.name)); |
There was a problem hiding this comment.
When %APPDATA% is unavailable, the directory is read-only, the disk is full, or exporting/writing the blob otherwise fails, this branch only logs a warning and NCryptFinalizeKey still returns success. The caller therefore believes its named key is persistent, but the next process receives NTE_BAD_KEYSET and generates a new identity—the exact session-churn behavior this patch is intended to prevent. Propagate the storage error instead of reporting successful finalization.
Useful? React with 👍 / 👎.
| + switch (algid) | ||
| + { | ||
| + case ECDSA_P256: bitlen = 256; break; | ||
| + case ECDSA_P384: bitlen = 384; break; | ||
| + default: bitlen = 1024; break; | ||
| + } | ||
| + set_object_property(object, NCRYPT_LENGTH_PROPERTY, (BYTE *)&bitlen, sizeof(bitlen)); | ||
| + set_object_property(object, BCRYPT_PUBLIC_KEY_LENGTH, (BYTE *)&bitlen, sizeof(bitlen)); |
There was a problem hiding this comment.
When a persisted RSA key was created after setting NCRYPT_LENGTH_PROPERTY to a non-default size such as 2048 or 4096 bits, the correct private blob is imported but this default arm unconditionally records its length as 1024. Subsequent NCryptGetProperty calls therefore return metadata inconsistent with the actual key, which can make consumers size signature buffers incorrectly or reject the reopened key. Recover the length from the imported BCrypt key or persist it in the header.
Useful? React with 👍 / 👎.
| + file = CreateFileW(path, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); | ||
| + if (file == INVALID_HANDLE_VALUE) goto done; |
There was a problem hiding this comment.
When NCryptCreatePersistedKey is called for an existing name without NCRYPT_OVERWRITE_KEY_FLAG, the implementation never checks the existing store and CREATE_ALWAYS silently replaces the established key during finalization instead of returning NTE_EXISTS. Concurrent launcher starts or any caller retrying creation can consequently rotate the device identity and invalidate the persistence guarantee; only explicitly requested overwrites should truncate an existing key.
Useful? React with 👍 / 👎.
|
Thanks, all three were real. Fixed in a0d933c. Storage failures now come back from NCryptFinalizeKey. You are right that a warning was the wrong call: a caller told its named key is finalized has no reason to look again, and would quietly get a new identity next run, which is the failure this patch exists to stop. Returning the error puts the launcher back on the pre-patch path it already handles (it logs the DPoP failure and continues unsigned) instead of hiding the problem. NTE_EXISTS unless NCRYPT_OVERWRITE_KEY_FLAG is passed. This one mattered more than it looks here: the Epic launcher runs two EpicOnlineServicesUserHelper processes alongside the main one, so concurrent creates are not hypothetical and one could have rotated the other's device key. The flag is not in wine's ncrypt.h, so the patch defines it (0x00000080) if it is missing. Key length survives a reopen. Reading it back with Verification after the changes: ncrypt conformance tests still 436 executed / 176 todo / 0 failures, and the P-256 key still loads from disk across runs. One update on the original problem: the launcher now runs without |
mikey92
changed the title
|
Added a second fix to this PR (b5587bb), found while looking at why GOG GALAXY goes unresponsive if you leave it running. What happens. The bottle's wineserver was gone while GalaxyClient stayed up for a day and a half. Sampling the process shows six threads stopped at the same place: Closing a handle needs the server, so the first thread waits forever and everything else piles up behind the fd cache lock it holds. GOG's own log agrees: the UI thread's last line is 11:12:04, while background threads on plain timers keep logging every minute to this day. The window is dead, the process looks fine. The signout follows from it too, since the 45 minute token refresh ran on that thread and never fired again. Why the server went missing. Fix. Battle.net's kill is scoped like the other three, and the reaper now checks each cycle that its own bottle still has a wineserver. Wine names the socket directory after the prefix's device and inode and the server runs with it as its working directory, so Verified against the real thing: the wedged GOG from 1d14h earlier was cleared 40 seconds after attaching the new reaper, all five processes gone. |
…ey reports The key-release patch (94847cc) is in the shipped winemac.so, yet Hogwarts Legacy still ends up with W held down, now together with a dead keyboard while the mouse keeps working. That pairing means Wine's keyboard focus moved to a window that is not the game's without the game being deactivated, which the patch cannot see because it only fires on macOS app deactivation. The window that lives inside the game's process is the EOS overlay (EOSOVH-*-Shipping.dll plus a CEF renderer tree). Refuse to load it, the way the Steam bottle already refuses gameoverlayrenderer; Wine applies the override to a full-path load as well (checked with regsvr32). SOJU_EPIC_OVERLAY=1 restores it. SOJU_KEYLOG=1 turns on +key,+event into ~/.battlenet-macos/logs so the next report shows which hwnd a KEY_RELEASE went to.
…path too lsof reports a resolved path, and /tmp is a symlink to /private/tmp on macOS, so the "is our wineserver alive" check never matched and every launcher was reaped as an orphan 40 s after starting. The GOG case that motivated the check really had no server, which is why it passed. Verified: Epic launcher started from its app bundle, still up with reaper and wineserver after two minutes; the EOS overlay override from the previous commit shows in its log as ERR_LOADLIBRARY and no EOSOverlayRenderer process appears.
… false orphan warning `soju` computed ROOT from BASH_SOURCE without resolving the symlink Homebrew puts in bin/, so ROOT became the brew prefix and every command except help failed with "install.sh: No such file". Resolve links first. The app bundles also baked the versioned Cellar path in; use opt/soju so brew upgrade does not orphan them. doctor used pgrep -c, which macOS pgrep lacks, so the wineserver count was always empty and doctor warned about orphaned services whenever a launcher was up. Count lines instead. New wrapper commands `soju gptk` and `soju sweep`, and every remediation message now names a command that exists for curl|bash users instead of a relative scripts/ path. install.sh fails loudly when the engine does not start rather than continuing into bottle creation; start_reaper returns 0 so a reaper without +x cannot abort Battle.net.app under set -e.
…windows Three of the reaper's tests could kill a live game. Window detection used OnScreenOnly, so a minimized or Cmd-H'd game, or one on another Space, was "windowless" and shot after 40 s. Idleness was a hard-coded list (D2R.exe, the launcher exe), so Hearthstone started from Battle.net with the launcher closed, or Hogwarts with Epic quit from its tray, brought the whole prefix down. And the kill patterns were unanchored, matching `tail -f D2R.exe.log` or the same exe in another bottle. Every Wine process, builtin services included, keeps a tmpmap file open in its wineserver's socket directory, which is named after the prefix, so `lsof +d` on that directory yields exactly this bottle's processes (15 of them in 90 ms). The bottle is idle only when nothing but the service set and the launcher's own helpers remains. Windows are enumerated with kCGWindowListOptionAll, ignoring Wine's 1x1 transparent placeholders; a game must be windowless for three checks. exe names are anchored on the path separator and the end of the word. Verified against the running Epic bottle: 15 processes attributed, the launcher alone classed as alive, helpers and services excluded, the launcher's off-screen window counted; the new reaper left the launcher alone past the old 40 s mark.
… swap a running engine An interrupted engine extract left bin/wine behind, and every re-run then skipped the engine as "already present". Extract into a staging directory and only count an engine that starts. Every download wrote straight to its final name, so a truncated file was taken as complete forever; write to .part and rename. update.sh ran rsync --delete over any directory without a .git *directory*, which includes worktrees and zip downloads with local edits: only the installer's own copy is replaced now, by directory swap rather than by copying over the running script. The engine is no longer swapped while a wineserver is up, since new processes would load the new engine against the old server. uninstall.sh now offers build/, *-support/, logs and update leftovers, so a full uninstall actually empties the base directory.
…fix in the clean env setup-steam-games.sh edited user.reg on disk right after reg add left a wineserver up with a dirty HKCU; the server rewrote the file on exit and put DISABLEDXMAXIMIZEDWINDOWEDMODE back, so games stayed forced fullscreen. Wait for the server first. play.sh had the same race in reverse for the AllowImmovableWindows line, now appended before anything boots the prefix, and its reg delete ran with the CX engine environment, so the process that booted the Steam prefix's wineserver carried DYLD_FALLBACK_LIBRARY_PATH, WINEMSYNC and the CX_* variables the launch line took care to unset.
…ay deletions ncrypt wrote the key file in place with CREATE_ALWAYS, so a crash or full disk mid-write left a truncated file that NCryptOpenKey rejected (NTE_BAD_KEYSET) while NCryptCreatePersistedKey saw it as existing (NTE_EXISTS): Epic could never get a device key again without someone deleting the file. Write to a .tmp beside it and rename over; and key_store_exists now reads the header and checks the size, so anything unreadable is treated as absent and gets replaced. The chromium-flags hook turned a deletion of QTWEBENGINE_CHROMIUM_FLAGS (NULL value in kernelbase, "NAME=" through the CRT) into a set of the Soju flags alone; leave deletions alone. releasePressedKeys cleared the driver's held-key record even when there was no front window to send the releases to, so the real key up that followed was dropped as a release of an unpressed key. Keep the record in that case. The three patches were regenerated as diffs against wine-11.0 sources with the edits applied; they apply cleanly and reproduce the edited tree. Not compiled here: an engine rebuild is needed before release.
…ean orphans by prefix soju-sweep refused to run while any wineserver was up, so the services of a bottle whose server had died lingered whenever another bottle was in use. It now removes only service processes attached to no running server (each live bottle's processes hold a file open in their server's socket directory), which makes it safe at any time; SOJU_SWEEP_DRY=1 previews. The reaper's orphan cleanup used to kill by launcher name; it now kills whatever holds a file open under the prefix, then sweeps. uninstall.sh removed every soju-named bundle in ~/Applications regardless of SOJU_BASE; it now only takes bundles whose launcher script names this install's engine. QA on a throwaway prefix: with notepad running the bottle stayed up; after quitting it the bottle came down within 50 s with nothing left; with its wineserver SIGKILLed, the reaper removed notepad and all nine services within 50 s while another bottle stayed untouched. Dry sweep with two live bottles lists nothing.
|
Nine more commits from a full bug review of the repo (three reviewers over scripts, installer/lifecycle, and the Wine patches), each finding verified against the code before fixing. The PR now also carries: Epic (351ab72): the EOS overlay is disabled by default ( Reaper (6ab0d04, f3d6b50, 3ab7578): the orphan check in b5587bb compared Homebrew and doctor (b82fa16, plus BCD1210/homebrew-soju@daf9b3f): install/update/uninstall (d2cef2c): atomic engine extract, Steam (2ed4bef): user.reg edits waited on the wineserver; the prefix boots in the clean env. Patches (a060c01): ncrypt writes keys atomically and treats an unreadable key file as absent (a truncated file used to wedge Epic on NTE_EXISTS); the chromium-flags hook keeps deletions as deletions; releasePressedKeys no longer clears its record when there is no window to deliver to. Regenerated as diffs against wine-11.0 and verified to apply and reproduce the edited tree, not compiled: an engine rebuild is needed before the next engine release. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41642e32b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pids=$(pgrep -f "$HELPER_RE" 2>/dev/null || true) | ||
| # shellcheck disable=SC2086 | ||
| [ -n "$pids" ] && kill -9 $pids 2>/dev/null || true |
There was a problem hiding this comment.
When another bottle is running the same launcher, this global pgrep selects its helpers too, so shutting down one Steam/Epic/GOG/Battle.net prefix can SIGKILL helper processes belonging to the other live prefix. The earlier classification is correctly based on the server directory, but that scope is discarded here; retain the helper PIDs from this bottle or filter them through prefix_pids before killing.
Useful? React with 👍 / 👎.
| + FlushFileBuffers(file)) | ||
| + ret = ERROR_SUCCESS; | ||
| + CloseHandle(file); | ||
| + if (ret == ERROR_SUCCESS && !MoveFileExW(tmp, path, MOVEFILE_REPLACE_EXISTING)) ret = NTE_INTERNAL_ERROR; |
There was a problem hiding this comment.
When two processes create the same previously absent named key before either finalizes it, both pass the creation-time existence check, and this unconditional MOVEFILE_REPLACE_EXISTING lets the later finalizer replace the first process's key even though neither requested NCRYPT_OVERWRITE_KEY_FLAG. Fresh evidence in this revision is that the new check remains separated from this replacing rename, leaving the original concurrent-launch scenario unresolved; atomically claim the destination or perform a no-replace finalization for such handles.
Useful? React with 👍 / 👎.
| + if (ReadFile(file, &header, sizeof(header), &read, NULL) && read == sizeof(header) && | ||
| + header.magic == KEY_STORE_MAGIC && header.blob_len && header.blob_len <= 0x10000 && | ||
| + size == sizeof(header) + header.blob_len) | ||
| + ok = TRUE; |
There was a problem hiding this comment.
When a key file is damaged without changing its header or total length—for example, a bit flip in the private blob—this predicate reports that the key exists even though BCryptImportKeyPair later rejects it. The caller then receives NTE_BAD_KEYSET from open and NTE_EXISTS from create, recreating the permanent wedge this recovery logic is intended to prevent; existence must include successful blob import or equivalent validation.
Useful? React with 👍 / 👎.
| + if (object->key.name && (path = key_store_path(object->key.name))) | ||
| + { | ||
| + DeleteFileW(path); | ||
| + free(path); | ||
| + } | ||
| + return NCryptFreeObject((NCRYPT_HANDLE)key); |
There was a problem hiding this comment.
When the persisted file cannot be deleted, such as because of permissions or a sharing violation, DeleteFileW fails but its result is ignored and NCryptDeleteKey returns the success from freeing the in-memory handle. The caller therefore believes the key was removed, while the next NCryptOpenKey silently reopens it; preserve and return the deletion error while still releasing the handle.
Useful? React with 👍 / 👎.
Concurrent creation: two launches creating the same absent key both passed the creation-time check, and the later finalize replaced the earlier key. The object now remembers whether NCRYPT_OVERWRITE_KEY_FLAG was given; a finalize without it renames the temp file without replacing, and answers NTE_EXISTS if a readable key got there first (an unreadable file is still replaced, so a damaged key cannot wedge it). Existence now means the stored key imports, not that its header and length look right, so a corrupted blob is treated as absent and recreated instead of failing open with NTE_BAD_KEYSET and create with NTE_EXISTS. NCryptDeleteKey returns the failure when the file will not go, rather than reporting success while the next open reopens the key. The reaper's idle teardown killed helpers by name across all bottles; it now kills only the helper PIDs its own bottle classification found.
|
Addressed all four: overwrite-aware no-replace finalize (NTE_EXISTS when a readable key won the race, damaged files still replaced), existence = successful import, NCryptDeleteKey reports a failed unlink, and the reaper's teardown now kills only the helper PIDs from its own bottle's classification. Patch regenerated and verified to apply; still not compiled. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36bd49b797
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| + { | ||
| + if (overwrite || GetLastError() != ERROR_ALREADY_EXISTS) ret = NTE_INTERNAL_ERROR; | ||
| + else if (key_store_exists(name)) ret = NTE_EXISTS; | ||
| + else if (!MoveFileExW(tmp, path, MOVEFILE_REPLACE_EXISTING)) ret = NTE_INTERNAL_ERROR; |
There was a problem hiding this comment.
When two no-overwrite creators finalize while the destination is damaged, one can validate the damaged file at line 161 and then another finalizer can rewrite the shared <path>.tmp or install a readable destination before this replacing rename runs. This finalizer can consequently overwrite the new winner or report success after moving the other handle's blob, so its in-memory public key differs from the key reopened later. Fresh evidence is the newly added validation-followed-by-MOVEFILE_REPLACE_EXISTING fallback, which remains a TOCTOU operation; remove or claim the damaged destination and then retry a no-replace rename atomically.
Useful? React with 👍 / 👎.
The no-overwrite finalize validated a file in place and then replaced it, which left a window in which another finalizer could install a good key that this one then destroyed, and two finalizers shared one <path>.tmp. Temp files now carry the process id, and a destination that blocks the no-replace rename is first moved aside under a per-process name (atomic, exclusive), examined there, and put back if it holds a readable key (NTE_EXISTS). Only a damaged file is discarded, then the no-replace rename is tried once more.
|
Addressed: temp files are per-process, and a blocking destination is moved aside under a per-process name before it is examined, so nothing is validated in place and then replaced. Put back if readable (NTE_EXISTS), discarded only if damaged, then the no-replace rename is retried once. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f20766234f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| + | ||
| + if (!(aside = malloc((lstrlenW(path) + 24) * sizeof(WCHAR)))) return NTE_NO_MEMORY; | ||
| + swprintf(aside, lstrlenW(path) + 24, L"%s.%u.aside", path, (unsigned int)GetCurrentProcessId()); | ||
| + if (!MoveFileExW(path, aside, 0)) |
There was a problem hiding this comment.
When a no-overwrite handle encounters a readable key installed by another finalizer, this move temporarily removes that winning key from the public path. A concurrent creator can then observe the name as absent and successfully install its own key; the first finalizer's restore fails and line 313 deletes the original readable key, so a no-overwrite operation has replaced an established identity. Fresh evidence in this revision is that the blocker is now moved away even when it is valid; finalizers need synchronization that prevents another creator from claiming the vacated path while the blocker is examined.
Useful? React with 👍 / 👎.
| + * disk mid-write cannot leave a truncated key behind: that would fail to | ||
| + * open (NTE_BAD_KEYSET) yet count as existing (NTE_EXISTS) forever. */ | ||
| + if (!(tmp = malloc((lstrlenW(path) + 24) * sizeof(WCHAR)))) { ret = NTE_NO_MEMORY; goto done; } | ||
| + swprintf(tmp, lstrlenW(path) + 24, L"%s.%u.tmp", path, (unsigned int)GetCurrentProcessId()); |
There was a problem hiding this comment.
When two threads in one process finalize handles for the same name concurrently, both still use the identical <path>.<pid>.tmp: one thread can overwrite the other's completed blob before it is renamed, causing the first handle to report success while persisting the second handle's key and leaving the second finalization to fail because its temp file is gone. Fresh evidence in this revision is that the suffix changed from a shared path to a process ID, which separates processes but not concurrent handles within one process; use a per-finalization unique name instead.
Useful? React with 👍 / 👎.
…per finalization Moving a valid key aside to examine it vacated the name for a moment, in which another creator could install its own key and the restore then failed. Finalizers of a name now take <path>.lock (no sharing, delete-on-close, so it vanishes with the handle even on process death) and examine, discard-if-damaged and install under it. Temp names carry a per-process counter as well as the pid, so two handles finalized on different threads never share one.
|
Addressed both: no-overwrite finalizes now serialize on @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 704502c717
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| on = cf.CFDictionaryGetValue(w, kOn) | ||
| if on and cf.CFBooleanGetValue(on): | ||
| pids.add(pid); continue |
There was a problem hiding this comment.
When a windowless D2R process retains Wine's 1×1 transparent placeholder as an on-screen window, this branch adds its PID immediately and bypasses the alpha and bounds filters below. The reaper therefore resets that process's strike count on every pass and never kills the zombie it is intended to clean up; apply the placeholder checks before accepting on-screen windows.
Useful? React with 👍 / 👎.
A windowless game that still owns Wine's 1x1 transparent placeholder, and that placeholder reported on-screen, would have had its strike count reset every pass. Size and alpha are checked for every window now; only the layer test is limited to off-screen ones, since a full screen game sits above layer 0.
|
Addressed: size and alpha filters now run for every window; only the layer test is limited to off-screen windows (a full screen game sits above layer 0). @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 088718c135
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| + if (overwrite) | ||
| + return MoveFileExW(tmp, path, MOVEFILE_REPLACE_EXISTING) ? ERROR_SUCCESS : NTE_INTERNAL_ERROR; |
There was a problem hiding this comment.
When an overwrite finalization races a no-overwrite finalization for the same name, this early return bypasses the lock used below: the no-overwrite caller can acquire the lock, observe no readable destination, then the overwrite caller installs its key and the no-overwrite caller replaces it with MOVEFILE_REPLACE_EXISTING. Both operations report success, but the final key belongs to the caller that explicitly lacked overwrite permission; make overwrite finalizers participate in the same per-name serialization.
Useful? React with 👍 / 👎.
| DIRS=$(lsof -a -p "$SERVERS" -d cwd -Fn 2>/dev/null | sed -n 's/^n//p' | tr '\n' '|') | ||
| ATTACHED=$(lsof -a -p "$(echo "$CAND" | tr '\n' ',' | sed 's/,$//')" -Fpn 2>/dev/null | awk -v dirs="$DIRS" ' |
There was a problem hiding this comment.
When another bottle is live and either lsof invocation fails or returns no usable output, DIRS or ATTACHED becomes empty and the loop below classifies every candidate service as orphaned. Because soju *-kill invokes this sweep while other bottles may remain active, a transient attribution failure can SIGKILL the services of those live bottles; fail closed whenever the server directories or candidate attachment query cannot be obtained.
Useful? React with 👍 / 👎.
…s closed An overwrite finalize bypassed the lock, so a no-overwrite finalizer could observe an empty destination, lose the race to the overwriter, and then replace its key anyway. Every finalize now runs under the lock; only the "is a readable key already there" test is skipped for overwriters. soju-sweep treated an empty attribution result as "everything is orphaned", which is exactly how a transient lsof failure would kill the services of a live bottle. With servers up, it now refuses to act unless it obtained both the server directories and an open-file listing for the candidates.
|
Addressed both: every finalize takes the per-name lock (overwriters just skip the readable-key test under it), and the sweep exits without acting when it cannot obtain the server directories or the candidates' open-file listing while servers are up. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8d0de69a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| printf '%s\n' "${TAG:-unknown}" > "$ENGINE.new/.soju-engine-release" | ||
| if [ -d "$ENGINE" ]; then | ||
| # A half-extracted engine from an earlier run; keep any GPTK payload in it. | ||
| [ -d "$ENGINE/lib/external" ] && cp -Rf "$ENGINE/lib/external" "$ENGINE.new/lib/" 2>/dev/null || true |
There was a problem hiding this comment.
When this recovery path replaces a broken engine that already contains the GPTK payload, it copies only lib/external into the freshly extracted engine and does not recreate the required d3d10.so, d3d11.so, d3d12.so, and dxgi.so symlinks. GPTK_OK then sees libd3dshared.dylib and skips install_gptk, leaving the repaired installation with the prebuilt engine's files instead of the D3DMetal links; games can consequently fail to start or use the wrong graphics backend. Restore the links here as scripts/update.sh does.
Useful? React with 👍 / 👎.
| [ -n "$SERVER_DIR" ] || return 0 | ||
| cand=$(pgrep -f '\.exe' 2>/dev/null | tr '\n' ',' | sed 's/,$//') | ||
| [ -n "$cand" ] || return 0 | ||
| lsof -a -p "$cand" +d "$SERVER_DIR_REAL" -Fp 2>/dev/null | sed -n 's/^p//p' | sort -un |
There was a problem hiding this comment.
When this lsof query transiently fails while a launcher or game is still running, the trailing sort makes the pipeline return success with no PIDs. The main loop consequently treats the live bottle as containing only idle services and, after two checks, invokes wineserver -k, terminating the user's session. Propagate an attribution failure separately from a genuinely empty result and skip that reaper round.
Useful? React with 👍 / 👎.
… a round on failed attribution The repair path copied lib/external into the fresh engine but not the d3d10/d3d11/d3d12/dxgi symlinks into it, so GPTK_OK passed and the engine kept the prebuilt .so files instead of the D3DMetal links. The reaper read an empty bottle listing as "only services left" after two rounds; since a live server always has its services attached, an empty answer is a failed lsof, and the round is skipped.
|
Addressed both: the engine repair path recreates the four D3DMetal symlinks alongside the copied payload, and the reaper treats an empty bottle listing as a failed attribution (a live server always has its services attached) and skips the round. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4efc54bff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [ -n "$SERVER_DIR" ] || return 0 # cannot tell, assume it is there | ||
| pids=$(pgrep -x wineserver 2>/dev/null | tr '\n' ',' | sed 's/,$//') | ||
| [ -n "$pids" ] || return 1 | ||
| lsof -a -p "$pids" -d cwd -Fn 2>/dev/null | grep -qxF -e "n$SERVER_DIR" -e "n$SERVER_DIR_REAL" |
There was a problem hiding this comment.
If this lsof invocation fails or returns no usable output for two consecutive rounds while the wineserver is still live, server_alive reports false and the orphan path calls reap_orphans, SIGKILLing the live bottle's processes. The new empty-BOTTLE guard does not cover this earlier attribution query; capture the lsof failure separately and skip the round rather than treating it as proof that the server disappeared.
Useful? React with 👍 / 👎.
server_alive returns 2 when lsof gives no output at all (every wineserver has a cwd, so that is the query failing, not the server gone), and the loop skips the round instead of counting an orphan strike.
|
Addressed: server_alive distinguishes a failed query (no lsof output at all, rc 2) from a missing server, and the loop skips that round. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96714c1ca8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| server_alive() { | ||
| local pids out | ||
| [ -n "$SERVER_DIR" ] || return 0 # cannot tell, assume it is there | ||
| pids=$(pgrep -x wineserver 2>/dev/null | tr '\n' ',' | sed 's/,$//') |
There was a problem hiding this comment.
When the active runtime exposes its server as wineserver64—a process name already handled by scripts/soju-sweep.sh:26—this exact-name query cannot find it (pgrep --help defines -x as matching the command name exactly). Consequently server_alive returns 1, and after two rounds the reaper kills processes from a live bottle; gather both wineserver and wineserver64 PIDs before concluding that the server is gone.
Useful? React with 👍 / 👎.
1 participant
Epic logins were failing with "your account has too many active logins", and neither signing out everywhere nor a password reset cleared it. The launcher log says what the screen does not:
Issued, not held. It is a rate limit on token issuance, so retrying feeds it and revoking sessions does nothing.
Why we were hitting it
Every one of the 17 launcher logs in the bottle starts with:
DPoP (RFC 9449) binds a token to a key the machine keeps. Without it the account service sees a new device on every start and issues a fresh session instead of refreshing the stored one, so ten launches burn ten sessions. On real Windows this is one session.
0x80090029 is NTE_NOT_SUPPORTED, and wine's ncrypt says why in its own source:
NCryptCreatePersistedKeyprintsFIXME("Persistent keys are not supported")and drops the name it was given,NCryptOpenKeyis a stub, and this version handles RSA only, so the ECDSA P-256 key DPoP wants is refused outright. Measured in the engine:The patch
Named keys now live where Windows keeps them,
%APPDATA%\Microsoft\Crypto\Keys, written when the key is finalized and read back byNCryptOpenKey. Opening a name that was never stored returnsNTE_BAD_KEYSETso callers create one rather than giving up. ECDSA P-256 and P-384 join RSA, since a key store that cannot hold the curve DPoP uses would not help.Testing
Probe run twice against the patched engine:
The identical public key on the second run is the point: it came off disk rather than being regenerated.
Regression: wine's own ncrypt conformance tests, stock vs patched, both 436 tests / 176 todo / 0 failures. Battle.net still boots on the patched engine (BREAKPOINT 0, no ncrypt errors). Both architectures built and installed; the engine keeps
.bak-origcopies.Not yet verified end to end against an Epic login: the account is still inside the issuance window, and attempting it would refill the counter. That check has to wait for the window to clear.