fix(net): Campaign LA LA7a review fixes — AD-97 register row, corrected restore justification
The Opus retail-lens review decoded the PDB-paired binary at CPlayerSystem::RestoreCharacter@0x0055d760 and refuted the uninitialized-edx justification: the two extra arguments are real push imm32 of a constant PStringBase (BN mis-renders them, but they pack to >=4 bytes each), so retail 0xF7D9 is >=16 bytes where ours is 8. The guid-only CODE stands (ACE reads only the guid; holtburger consensus) but it is an adaptation, not a corrected decompile — filed as divergence register AD-97 and the doc comment now states the true mechanism. Also from the review: the 0xF643 conditional-parse doc now names BOTH ACE flag-only failure branches (NameInUse + Corrupt); CharacterError 0x08 doc corrected (ACE misnames it ServerCrash2 — the port corrects an ACE misnaming; ACE omits three values, not four); LA7b hazard notes added (ACE silent no-reply on unknown restore guid; retail SendToLogon vs SendToControl routing; NumErrors never rendered); two review-nit tests (flag=0 Undef flag-only, non-Ok body with trailing bytes ignored). Core.Net suite: 953 passed / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
6a32f37589
commit
4338b1c1f3
5 changed files with 100 additions and 34 deletions
|
|
@ -189,6 +189,7 @@ readiness/requeue adaptation. See
|
|||
| AD-92 | **Filed 2026-08-13 at the #376/#388 review fix round (blast M6 / mechanism M4).** Two switcher adaptations with no retail counterpart: (1) the fullscreen refresh rate is the monitor's HIGHEST for the picked WxH — retail passed the device mode's own refresh as-is (`Device::ForceDisplayResolution`); (2) an invalid/unsupported fullscreen request is a logged refusal that leaves the window unchanged — retail attempted the switch and surfaced the device error. The persisted-flag divergence a refusal leaves behind is ISSUES #392. | `src/AcDream.App/Settings/DisplayModeSwitching.cs` (`TryFindRefreshRate`, the refusal paths); `src/AcDream.App/Settings/RuntimeSettingsTargets.cs` (`Apply`'s refused-mode logging) | Highest-refresh is strictly better on modern variable-refresh panels (retail predates them); refuse-and-log is #388's own no-crash requirement. | A capture comparing retail's exact chosen refresh for a mode will differ; a server/tooling flow expecting an error dialog on an invalid mode sees a console line instead. | `Device::ForceDisplayResolution @gmClient::Init 0x004047af`; docs/research/2026-08-13-376-388-{mechanism,blast}-review.md |
|
||||
| AD-94 | **Filed 2026-08-14 at the secure-trade feature.** Retail's `Event_AcceptTrade` payload (`Trade::Pack @0x005B9FF0`) appends two `PackableList<ContentProfile>` staged-item lists after the six fixed fields; acdream sends both as ZERO-COUNT lists. ACE parses and then discards the ENTIRE payload (`HandleActionAcceptTrade()` takes zero arguments — server trade state is fully self-derived; lane B §quirks), so the difference is unobservable against ACE; a byte-capture comparison against a real retail client would differ from offset 40. | `src/AcDream.Core.Net/Messages/TradeRequests.cs` (`BuildAcceptTrade`) | The `ContentProfile` pack layout was not byte-verified (ACE never reads it — no reader to check against), and guessing a wire struct violates the workflow; zero-count lists are well-formed `PackableList`s. | A future server that actually validates the accept echo would see empty item lists and could refuse or desync the accept. | `Trade::Pack @0x005B9FF0`; `GameActionAcceptTrade.cs:11-16`; `docs/research/2026-08-14-trade-laneB-wire.md` Table 1 |
|
||||
| AD-96 | **Filed 2026-08-14 at the OP8 re-gate fix round (key-name display).** Retail's `GetNameFromKey_Internal @0x00687800` falls back from the DAT string tables (key enum 4 → `0x2300000A`, meta enum 5 → `0x2300000B`) to the OS keyboard layout's own key name via DirectInput `IDirectInputDevice8::GetObjectInfo` (`tszName` — "SKIFT" on a Swedish layout). acdream reads the SAME layout-resident name data through Win32 `GetKeyNameTextW` instead (no DirectInput device exists in-process); on non-Windows hosts there is no OS lookup at all and the DIK-suffix spelling shows (un-localized English, e.g. "LSHIFT"). Mouse chords keep the pre-existing enum spelling — retail names them through the DirectInput mouse device. | `src/AcDream.App/Platform/PlatformKeyNameProvider.cs`; `src/AcDream.App/UI/Layout/RetailKeyNames.cs` (`Describe`, the mouse-device early-out) | GetKeyNameText and DirectInput's key names both come from the active keyboard-layout tables; adding a DirectInput device solely for name strings would be a heavyweight, dead-end dependency. Linux graphical work is parked at Slice L1. | A key whose GetKeyNameTextW name differs from DirectInput's `tszName` on some layout shows a slightly different caption than retail did; Linux graphical shows English DIK-suffix names where retail-on-Wine would localize; a mouse-chord caption reads as the Silk enum, not retail's device string. | `CInputManager_WIN32::GetNameFromKey_Internal @0x00687800`; `GetNameFromKey @0x00687F40`; `ControlSpecification::GetDIKName @0x0068ACB0`; `DBCache::GetDIDFromEnumStatic` category-4 probe 2026-08-14 (`KeyboardConfigLiveMountProbeTests.ProbeKeyboardFontsAndKeyNameStrings`) |
|
||||
| AD-97 | **Filed 2026-08-14 at Campaign LA slice LA7a (character-restore request tail).** Retail's `CharacterRestore` request (`0xF7D9`) is ≥16 bytes: `CPlayerSystem::RestoreCharacter @0x0055d760` is, in the PDB-paired binary, `push 0x008173B4; push 0x008173B4; push guid; call Proto_UI::SendAdminRestoreCharacter @0x00546cf0`, and the callee packs BOTH constant `PStringBase<char>*` arguments (`PStringBase::Pack @0x004fc6f0` emits ≥4 bytes even empty). Binary Ninja renders the two pushes as an uninitialized `edx` local plus `this` — a rendering artifact around constant `0x008173B4` (4 of its 5 other pseudo-C appearances sit in provably-broken decompiles), but the arguments are real. acdream sends the 8-byte guid-only form. What the two constant strings contain is unresolved (a live cdb `db poi(0x008173b4)` would settle it). | `src/AcDream.Core.Net/Messages/CharacterRestore.cs` (`BuildRequestBody`) | ACE reads only `ReadUInt32()` and ignores any tail (`CharacterHandler.cs:331-385`), and holtburger ships guid-only from a real client command path against ACE successfully — the tail is unread by every server we can test against, and packing two strings whose CONTENT we cannot verify would be a guess. | A byte-capture comparison against a real retail client differs from offset 8; a future server that validates the full retail shape would reject our 8-byte request. | `CPlayerSystem::RestoreCharacter @0x0055d760` (binary bytes, not the BN rendering); `Proto_UI::SendAdminRestoreCharacter @0x00546cf0`; `PStringBase::Pack @0x004fc6f0`; ACE `CharacterHandler.cs:331-385`; holtburger `character_selection.rs:79-82`; LA7a Opus review F1 (2026-08-14) |
|
||||
| AD-93 | **Filed 2026-08-13 at social gate round 2, item 5 (the refused-drop notice port).** Two narrow gaps in the `ServerSaysAttemptFailed @0x0058EAE0` port: (1) **latched-guid preference** — retail's 0x00A0 dispatcher (`@0x0055B342`) PREFERS `prevRequestObjectID` over the wire guid when picking the item to name; acdream's `InventoryTransactionState.OnMoveFailed` instead REQUIRES the wire guid to match the latch (unobservable against ACE, which always sends the request's own guid on 0x00A0, and it protects a stale latch from mislabeling an unrelated failure — acdream has no retail-style latch timeout). (2) **unlatched request kinds** — retail latches `IR_MOVE`/`IR_WIELD` too; acdream's kind enum has no Move/Wield rows because wields ride `AutoWieldController` outside the single-request gate, so a refused wield/3D-move shows only the generic `HandleFailureEvent` leg, never "The X can't be wielded/moved". | `src/AcDream.Core/Items/InventoryTransactionState.cs` (`OnMoveFailed`); `src/AcDream.Core/Chat/InventoryFailureMessages.cs` (`Compose`'s absent Move/Wield rows); `src/AcDream.App/UI/ItemInteractionController.cs` (`OnInventoryRequestFailed`) | The match requirement is the compensating guard for the missing latch timeout; adding Wield/Move kinds means routing those sends through the single-request gate they deliberately bypass today — a behavior change beyond this gate item. | Only observable against a server that sends 0x00A0 with a guid that differs from the request's item (ACE never does), or on a refused wield/move, which shows no "can't be wielded/moved" verb line where retail would show one. | `ACCWeenieObject::ServerSaysAttemptFailed @0x0058EAE0`; the 0x00A0 dispatcher `@0x0055B342`; `ACCWeenieObject::RecordRequest @0x0058C220`; `docs/research/2026-08-13-confirm-and-weenie-error-display.md` §2 |
|
||||
|
||||
---
|
||||
|
|
|
|||
|
|
@ -43,6 +43,13 @@ namespace AcDream.Core.Net.Messages;
|
|||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// Routing note for LA7b: retail transmits this request via
|
||||
/// <c>Proto_UI::SendToLogon</c> (the restore request rides
|
||||
/// <c>SendToControl</c>); ACE sends its acknowledgement and the follow-up
|
||||
/// refreshed CharacterList on <c>GameMessageGroup.UIQueue</c>.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// After the ack, ACE immediately follows with a fresh <see cref="CharacterList"/>
|
||||
/// so the roster reflects the character's new pending-delete state
|
||||
/// (<c>CharacterHandler.CharacterDelete</c>,
|
||||
|
|
|
|||
|
|
@ -43,11 +43,15 @@ namespace AcDream.Core.Net.Messages;
|
|||
/// It is a strict superset of ACE's <c>ACE.Server.Network.Enum.CharacterError</c>
|
||||
/// (<c>references/ACE/Source/ACE.Server/Network/Enum/CharacterError.cs</c>):
|
||||
/// retail additionally names 0x2 (<c>LoggedOn</c>), 0x7 (<c>NoPremade</c>),
|
||||
/// 0x8 (<c>AccountInUse</c>), and 0x16 (<c>CharacterIsBooted</c>), none of
|
||||
/// which ACE's server ever sends but all of which retail's client can
|
||||
/// receive from a genuine retail server — per the project's
|
||||
/// property-enum-divergence lesson, we port the complete oracle, not just
|
||||
/// what today's one server implementation emits. ACE's per-value doc
|
||||
/// and 0x16 (<c>CharacterIsBooted</c>) — three values ACE omits entirely,
|
||||
/// none of which ACE's server ever sends but all of which retail's client
|
||||
/// can receive from a genuine retail server. At 0x8 the port additionally
|
||||
/// CORRECTS an ACE misnaming: ACE defines 0x8 as <c>ServerCrash2</c> with a
|
||||
/// doc comment duplicating 0x4's <c>ID_CHAR_ERROR_SERVER_CRASH</c> text,
|
||||
/// but retail's header names 0x8 <c>CHAR_ERROR_ACCOUNT_IN_USE</c> — the
|
||||
/// header wins. Per the project's property-enum-divergence lesson, we port
|
||||
/// the complete oracle, not just what today's one server implementation
|
||||
/// emits. ACE's per-value doc
|
||||
/// comments (themselves sourced from the client's <c>ID_CHAR_ERROR_*</c>
|
||||
/// string table) are folded in below where they exist. One retail member,
|
||||
/// <c>FORCE_charError_32_BIT = 0x7FFFFFFF</c>, is a compiler
|
||||
|
|
@ -111,7 +115,11 @@ public static class CharacterError
|
|||
/// <summary>0x07 — CHAR_ERROR_NO_PREMADE. Retail-only; no ACE member.</summary>
|
||||
NoPremade = 0x07,
|
||||
|
||||
/// <summary>0x08 — CHAR_ERROR_ACCOUNT_IN_USE. Retail-only; no ACE member.</summary>
|
||||
/// <summary>
|
||||
/// 0x08 — CHAR_ERROR_ACCOUNT_IN_USE. ACE misnames this value
|
||||
/// <c>ServerCrash2</c> (its doc comment duplicates 0x04's text);
|
||||
/// retail's header is the authority. See the class doc comment.
|
||||
/// </summary>
|
||||
AccountInUse = 0x08,
|
||||
|
||||
/// <summary>
|
||||
|
|
@ -228,7 +236,8 @@ public static class CharacterError
|
|||
/// sentinel (the array-bound idiom, one past the last real code) —
|
||||
/// never sent on the wire as an actual error. Kept for verbatim
|
||||
/// completeness of the enum range; do not treat a received 0x19
|
||||
/// as meaningful.
|
||||
/// as meaningful, and LA7b's error-to-string mapping must not
|
||||
/// render it as a user-facing message.
|
||||
/// </summary>
|
||||
NumErrors = 0x19,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -8,27 +8,36 @@ namespace AcDream.Core.Net.Messages;
|
|||
/// (opcode <c>0xF643</c>).
|
||||
///
|
||||
/// <para>
|
||||
/// <b>Request — guid-only, by reference consensus.</b> The decompiled call
|
||||
/// site (<c>Proto_UI::SendAdminRestoreCharacter</c> at <c>0x00546cf0</c>,
|
||||
/// declared with three parameters — a u32 and two <c>PStringBase<char></c>
|
||||
/// pointers — and packing two strings after the u32) LOOKS like it sends
|
||||
/// guid + two strings. It does not: its only real caller,
|
||||
/// <c>CPlayerSystem::RestoreCharacter</c> at <c>0x0055d760</c>, declares
|
||||
/// <c>class PStringBase<char>* edx;</c> as a local and passes it
|
||||
/// straight through UNINITIALIZED as the second argument, and passes
|
||||
/// <c>this</c> (a <c>CPlayerSystem*</c>, not a string) as the third. Both
|
||||
/// are textbook decompiler register-corruption artifacts (uninitialized
|
||||
/// register reuse + a mistyped extra parameter from an over-declared
|
||||
/// callee signature), not real arguments the real call site ever
|
||||
/// supplied. ACE
|
||||
/// <b>Request — guid-only, an ADAPTATION (register row AD-97).</b> Retail
|
||||
/// really does send more than the guid. The PDB-paired binary at
|
||||
/// <c>CPlayerSystem::RestoreCharacter@0x0055d760</c> is 26 bytes:
|
||||
/// <c>push 0x008173B4; push 0x008173B4; push guid;
|
||||
/// call Proto_UI::SendAdminRestoreCharacter@0x00546cf0</c> — two REAL
|
||||
/// constant <c>PStringBase<char>*</c> arguments (Binary Ninja renders
|
||||
/// them as an uninitialized <c>edx</c> local and <c>this</c>; that
|
||||
/// rendering is the artifact, the two <c>push imm32</c> are not).
|
||||
/// <c>SendAdminRestoreCharacter</c> packs both
|
||||
/// (<c>PStringBase::Pack@0x004fc6f0</c> emits ≥4 bytes even for an empty
|
||||
/// string), so retail's request is ≥16 bytes where ours is 8. We send
|
||||
/// guid-only because ACE
|
||||
/// (<c>CharacterHandler.CharacterRestore</c>,
|
||||
/// <c>ACE.Server/Network/Handlers/CharacterHandler.cs:331-385</c>, reads
|
||||
/// only <c>ReadUInt32()</c>) and holtburger
|
||||
/// <c>ACE.Server/Network/Handlers/CharacterHandler.cs:331-385</c>) reads
|
||||
/// only <c>ReadUInt32()</c> and ignores any tail, and holtburger
|
||||
/// (<c>holtburger-protocol/src/messages/character/types.rs::CharacterRestoreRequestData</c>,
|
||||
/// guid-only) independently agree on guid-only. We follow the two
|
||||
/// independent, uncorrupted references (design spec §11 item 4 — wire
|
||||
/// consensus, no divergence-register row needed: this isn't a deviation
|
||||
/// from retail, it's picking the correct reading of a corrupted decompile).
|
||||
/// sent from a real client command path) ships guid-only against ACE
|
||||
/// successfully. The omitted tail is a recorded retail deviation —
|
||||
/// divergence register AD-97.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// <b>LA7b hazards.</b> (1) ACE's restore handler has a SILENT no-reply
|
||||
/// path: an unknown guid hits
|
||||
/// <c>Characters.SingleOrDefault(...) == null → return;</c> — no 0xF643,
|
||||
/// no 0xF659. Selection state must never await a restore reply
|
||||
/// unconditionally. (2) Routing: ACE sends the response on
|
||||
/// <c>GameMessageGroup.UIQueue</c>; retail transmits the request via
|
||||
/// <c>Proto_UI::SendToControl</c> (the delete request goes via
|
||||
/// <c>SendToLogon</c>) — relevant when LA7b picks the outbound queue.
|
||||
/// </para>
|
||||
///
|
||||
/// <code>
|
||||
|
|
@ -55,12 +64,13 @@ namespace AcDream.Core.Net.Messages;
|
|||
/// </code>
|
||||
///
|
||||
/// <para>
|
||||
/// But retail's <c>CharacterRestore</c> handler can ALSO reply on this same
|
||||
/// But ACE's <c>CharacterRestore</c> handler can ALSO reply on this same
|
||||
/// opcode via the character-CREATE response path when restore itself fails
|
||||
/// (e.g. <c>SendCharacterCreateResponse(session, CharacterGenerationVerificationResponse.NameInUse)</c>
|
||||
/// when the freed name collides) — that shape is flag-only, with NO
|
||||
/// trailing fields (<c>GameMessageCharacterCreateResponse.cs</c>: the guid /
|
||||
/// name / trailing u32 are only written <c>if (response == ... .Ok)</c>).
|
||||
/// — TWO real branches: <c>NameInUse</c> (the freed name collided) and
|
||||
/// <c>Corrupt</c> (<c>SaveCharacter</c> returned false). Both shapes are
|
||||
/// flag-only, with NO trailing fields
|
||||
/// (<c>GameMessageCharacterCreateResponse.cs</c>: the guid / name /
|
||||
/// trailing u32 are only written <c>if (response == ... .Ok)</c>).
|
||||
/// <see cref="Parse"/> mirrors that conditionality: the trailing three
|
||||
/// fields are read only when <c>verificationFlag == 1</c>. Because the two
|
||||
/// message families are wire-identical when they collide, a caller cannot
|
||||
|
|
@ -95,9 +105,9 @@ public static class CharacterRestore
|
|||
|
||||
/// <summary>
|
||||
/// Build the body bytes for an outbound <c>CharacterRestore</c> request.
|
||||
/// Layout: opcode(4) + characterGuid(4). Guid-only — see the class doc
|
||||
/// comment for why the decompiled call site's apparent extra strings
|
||||
/// are not real.
|
||||
/// Layout: opcode(4) + characterGuid(4). Guid-only — an adaptation of
|
||||
/// retail's ≥16-byte shape; see the class doc comment and divergence
|
||||
/// register AD-97.
|
||||
/// </summary>
|
||||
public static byte[] BuildRequestBody(uint characterGuid)
|
||||
{
|
||||
|
|
|
|||
|
|
@ -74,6 +74,45 @@ public sealed class CharacterRestoreTests
|
|||
Assert.Null(parsed.SecondsGreyedOut);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Parse_UndefFlagZero_FlagOnlyBody_LeavesTrailingFieldsNull()
|
||||
{
|
||||
// LA7a review test-coverage nit: flag 0 (Undef) is a non-Ok value
|
||||
// distinct from the NameInUse case — the conditional must treat it
|
||||
// as flag-only too.
|
||||
var w = AceWireWriter.GameMessage(CharacterRestore.ResponseOpcode)
|
||||
.Write(0u); // CharacterGenerationVerificationResponse.Undef
|
||||
|
||||
CharacterRestore.Parsed parsed = CharacterRestore.Parse(w.ToArray());
|
||||
|
||||
Assert.Equal(0u, parsed.VerificationFlag);
|
||||
Assert.False(parsed.IsOk);
|
||||
Assert.Null(parsed.Guid);
|
||||
Assert.Null(parsed.Name);
|
||||
Assert.Null(parsed.SecondsGreyedOut);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Parse_NonOkBodyWithTrailingBytes_IgnoresRatherThanMisreads()
|
||||
{
|
||||
// LA7a review test-coverage nit: a non-Ok body that DOES carry
|
||||
// trailing bytes (unknown server variant / padding) must not be
|
||||
// misread as character fields — the conditional stops at the flag
|
||||
// and the extra bytes are ignored.
|
||||
var w = AceWireWriter.GameMessage(CharacterRestore.ResponseOpcode)
|
||||
.Write(3u) // NameInUse
|
||||
.Write(0xDEADBEEFu)
|
||||
.Write(0x12345678u);
|
||||
|
||||
CharacterRestore.Parsed parsed = CharacterRestore.Parse(w.ToArray());
|
||||
|
||||
Assert.Equal(3u, parsed.VerificationFlag);
|
||||
Assert.False(parsed.IsOk);
|
||||
Assert.Null(parsed.Guid);
|
||||
Assert.Null(parsed.Name);
|
||||
Assert.Null(parsed.SecondsGreyedOut);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Parse_WrongOpcode_Throws()
|
||||
{
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue