Full-codebase audit: 74 verified bug, performance, and memory fixes (9.1.0)#116
Open
logicallysynced wants to merge 6 commits into
Open
Full-codebase audit: 74 verified bug, performance, and memory fixes (9.1.0)#116logicallysynced wants to merge 6 commits into
logicallysynced wants to merge 6 commits into
Conversation
The harness previously only surfaced enmity via TargetInfo.EnmityItems,
which Reader.Target.cs gates behind CurrentTargetID > 0 — so the table
was invisible when the player had no target. Two distinct enmity-related
structures exist on UIState and both deserve visibility for testing:
AGRO list (UIState.Hater._haters, HaterCount)
AGROMAP_KEY / AGRO_COUNT_KEY — "who is aggro'd on the player"
Already surfaced on PlayerInfo.EnmityItems; reprinted with the raw
counter alongside for verification.
HATE list (UIState.Hate._hateInfo, HateArrayLength)
ENMITYMAP_KEY / ENMITY_COUNT_KEY — "the player's own hate table"
Read directly from memory using HateItem offsets so the data is
visible regardless of whether GetTargetInfo would populate it.
Names looked up against GetActors() since hate entries store only
ID + Enmity.
Also logs CanGetAgroEntities / CanGetEnmityEntities capability flags so
a stripped scan (missing signature) is obvious at a glance, and lists
up to 10 actors with IsAgroed || InCombat set — independent of either
table — as an eye-check that ActorItem.IsAgroed lights up correctly.
The note above the TargetInfo.EnmityItems print is updated to point at
[3c.3] so it's clear why the same data shows up twice in different forms.
Harness-only — no Sharlayan source changes, no version bump.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Upstream rolled two commits since the previous pin: b21433ecc Add names from EventSystemDefine as comments e04ec6b37 Update structs Routine struct refresh + a comments-only addition. No public-API, field-rename, or layout-shape changes that Sharlayan would notice. Lumina 7.5.0, Lumina.Excel 7.4.3, NLog 6.1.3, Newtonsoft.Json 13.0.4, and ILRepack.Lib.MSBuild.Task 2.0.45 are already at latest stable (verified via `dotnet list package --outdated` — no available updates). Build clean, 189/189 tests pass, merged Sharlayan.dll byte size unchanged at 10980 KB, debug DLLs deployed to Chromatics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s, resolvers, and memory core Multi-agent audit of the full Sharlayan codebase (FCS submodule excluded), every finding adversarially verified before fixing. Highlights: - ActorItem/PartyMember.Clone() now copy StatusNameEnglish (was silently null) - GetEventObjectType receives the real actor address (was always IntPtr.Zero) - Inventory container stride derived from FCS Size (was hardcoded 24 vs actual 32) - Materia IDs read as ushort (Dawntrail grades > 255 no longer truncated) - RPR/SGE levels + EXP wired end-to-end in PlayerInfo (were always 0) - Party member Coordinate Y/Z typo; duplicate status adds; enmity NRE chain - GetActors clones only genuinely-removed actors (was deep-cloning everything per poll) - GetGameState derives IsLoggedIn from a pointer read (was running full player resolve) - MemoryHandler primitive getters use thread-static buffers (zero alloc per read) - Chat pipeline: bounds guards, compiled regexes, LINQ/copy churn removed - Scanner: per-region exception flood removed, signature bytes hoisted out of scan loop - P/Invoke hygiene: CloseHandle BOOL, GetStructure try/finally + correct handle - DEPENDENCY.md + FCS integrity tests synced (InventoryContainer.SourceSize, RPR/SGE) Build clean, 190/190 tests pass. Version 9.0.40. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ActionResult.ActionContainers / InventoryResult.InventoryContainers: ConcurrentBag<T> -> List<T> (populated sequentially; the bag's thread-local machinery was pure overhead per poll) - AstrologianResources.DrawnCards: List<AstrologianCard> -> AstrologianCard[] - DancerResources.Steps: List<DanceStep> -> DanceStep[] (built directly, no temp array); CurrentStep adapted, Steps defaults to empty - MonkResources.BeastChakra: stored array -> computed from BeastChakra1-3 (zero per-poll allocation; values identical on access) - InventoryItem materia arrays: shared zero-filled length-5 arrays for items with no melds; per-item arrays only when materia present - ActorItemResolver: fallback names for nameless actors cached per type id Minor version to 9.1 for the public API surface changes (authorized). Build clean, 190/190 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # Sharlayan/version.props
38 upstream commits (struct updates, WeatherInterface, TerritoryInfo/ MapRangeFlags updates, EventFramework.SetDirectorData). Build clean, 190/190 tests pass including the FCS dependency integrity suite - no Sharlayan-side changes needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the output of a full audit of the library: every reader, resolver, mapper, the process-memory core, the harness, and the tests. The FFXIVClientStructs submodule is external and was left alone. The sweep produced 114 raw findings; each one was re-verified against the actual code before any fix went in, which eliminated 16 false positives and merged the duplicates, leaving 74 confirmed issues. This PR fixes all 74.
Build is clean with zero warnings and all 190 tests pass, including a new integrity test that pins the inventory container stride to the FCS struct size. DEPENDENCY.md and the FCS integrity tests are synced with the new mapper fields. Version goes to 9.1.1: five fixes change public API surface, so this is a minor bump, not a patch, plus a patch tick for the submodule bump below.
This branch also merges current main (the earlier FCS bump and harness enmity-table work landed there as #115) and bumps the FFXIVClientStructs submodule to 15ae1806b - 38 upstream commits absorbed with no Sharlayan-side changes, which is the zero-touch offset-derivation doing its job.
Fixes that change observable output
Four corrections return real data where the old code returned wrong or empty values. Everything else in this PR preserves output exactly.
targetAddress(alwaysIntPtr.Zero) withcharacterAddressin theGetEventObjectTypecall so EventObject actors get their real type.SourceSizeproperty toSharlayan.Models.Structures.InventoryContainer, populated it viaMarshal.SizeOf<NativeInventoryContainer>()inInventoryContainerMapper.Build(), and replaced the hard-coded_inventoryByteCount = 24constant inReader.Inventory.cswiththis._memoryHandler.Structures.InventoryContainer.SourceSize(correct stride is 32, matching the FCS[StructLayout(Size=0x20)]declaration).Inventory.MateriaTypeenum from byte to ushort backing, so Dawntrail grade VIII/IX/X materia ids above 255 are preserved.RPR/SGE/RPR_CurrentEXP/SGE_CurrentEXPproperties and wired them end-to-end: structure offsets from ExpIdx 28/29 inPlayerInfoMapper, public properties onCore/PlayerInfoandIPlayerInfo, assignments inPlayerInfoResolver. Same pattern as VPR and PCT.Public API changes (the 9.1 part)
Five fixes touch types that consumers compile against. Chromatics needs a rebuild and two one-line tweaks (
DrawnCards.CountbecomesDrawnCards.Lengthin JobGaugeB and JobGaugeC); everything else it uses is shape-compatible.ActionResult.ActionContainersfromConcurrentBag<ActionContainer>toList<ActionContainer>. The reader populates it sequentially; the bag's thread-local machinery was overhead with no concurrent producer.InventoryResult.InventoryContainersfromConcurrentBag<InventoryContainer>toList<InventoryContainer>, same reasoning.MateriaTypes/MateriaRanksarrays; per-item arrays are allocated only when materia is present. Property types are unchanged; treat the arrays as read-only.TypeID: n/EventObjectTypeID: n) are cached per type id, so the interpolated string is built once per distinct id instead of once per nameless actor per poll.AstrologianResources.DrawnCardsis nowAstrologianCard[](wasList<AstrologianCard>),DancerResources.Stepsis nowDanceStep[]built directly without a temp array (CurrentStepadapted, defaults to empty), andMonkResources.BeastChakrais computed fromBeastChakra1/2/3on access instead of being allocated every poll.Bug fixes (output-preserving)
StatusNameEnglish = statusItem.StatusNameEnglish,to the StatusItem object initializer inside theforeach (StatusItem statusItem in this.StatusItems)loop inActorItem.Clone(), matching the existing field-copy pattern for all other StatusItem properties.StatusNameEnglish = item.StatusNameEnglish,to the StatusItem object initializer inside theforeach (StatusItem item in this.StatusItems)loop inPartyMember.Clone(), matching the existing field-copy pattern.if (_damageOverTimeActions.Any())andif (_healingOverTimeActions.Any())guards that dereferenced null on the first call toDamageOverTimeActions()andHealingOverTimeActions().int sepIdx = chatLogEntry.Line.IndexOf(INDEX_CHECK, ...)and guarded the Substring and Message.Replace calls insideif (sepIdx >= 0), so a colon-free public chat line flows through without throwing ArgumentOutOfRangeException.new Coordinate(entry.X, entry.Z, entry.Z)toentry.YatPartyMemberResolver.cs:81. The typo caused the Y component to equal Z for every party member resolved via the raw-bytes path.typeof(T), ignoring sqpack path - multi-instance data contaminationLuminaLookup._cachekey fromTypeto(Type, string)tuple where the string is the lower-invariant sqpack directory path resolved fromSharlayanConfigurationvia a newResolveSqpackKeyhelper (same logic asLuminaGameDataCache.TryResolveSqpackPath).new byte[4]tonew byte[2]inGetUInt16so the method reads exactly the 2 bytes it needs instead of over-reading across a potential page boundary.SharlayanBitConverter.TryToUInt32(value, 0)toSharlayanBitConverter.TryToUInt64(value, 0)inGetUInt64; the method was reading 8 bytes correctly but discarding the upper 32 bits by calling the wrong converter.ActionItem.IDassignment inReader.Actions.csfromSharlayanBitConverter.TryToInt16(hotbarMap, ...)to(int)SharlayanBitConverter.TryToUInt32(hotbarMap, ...), matching the FCSHotbarSlot.CommandIdfield width (uintat[FieldOffset(0xB8)]) so action IDs above 32767 are no longer truncated.this._memoryHandler.GetInt16(...)tothis._memoryHandler.GetInt32(...)and the local variable type fromshorttointforagroCount, matching the 4-byteint HaterCountfield declared in FFXIVClientStructs.IntPtr targetAddress = IntPtr.Zerodeclaration, theif (targetAddress.ToInt64() > 0)conditional block (always false, so TargetID was never read from that path), and thetargetInfoMapbuffer allocation and pool return.uint itemId <= 0comparison is always-false dead branchif (itemId <= 0)comparison (whereitemIdisuint) toif (itemId == 0)to express the empty-slot intent precisely and eliminate the dead< 0branch.short enmityCount = GetInt16(counter)toint enmityCount = GetInt32(counter)inReader.Target.cs, matching the 4-byteint HateArrayLengthfield declared in FCS.Marshal.OffsetOf<PlayerState>("_classJobLevels"),Marshal.OffsetOf<PlayerState>("_classJobExperience"),Marshal.OffsetOf<PlayerState>("_attributes")calls and theMarshal.SizeOf<PlayerState>()call inPlayerInfoMapper.Build()with the project-standardFieldOffsetReader.OffsetOf<PlayerState>(...)andFieldOffsetReader.SizeOf<PlayerState>()equivalents, matching every other mapper in the codebase.FFXIVClientStructsSignatureExtractor.BuildCache, changedcache[t.Name] = infotocache.TryAdd(t.Name, info)so the first[StaticAddress]method wins on short-name collision, and added abreakafter registering the first[StaticAddress]-decorated method per type so that a type with multiple such methods does not silently overwrite the short-name entry on each iteration.LiveRenderer.End(), saved_thisFrameLineCountintocontentLinesbefore the ghost-clearing loop, then assigned_previousLineCount = contentLines(instead ofMath.Max) so the high-water mark shrinks when the display gets shorter.using FFXIVClientStructs.FFXIV.Client.UI.Arrays;to the test file and replaced272 * sizeof(int)in theBuild_ContainerSize_MatchesActionBarBarNumberArraySizeassertion withMarshal.SizeOf<ActionBarNumberArray.ActionBarBarNumberArray>().this._foundStatuses.Add(statusEntry)at old line 116 ofPartyMemberResolver.cs. Only the single add inside theIsValid()block remains, matching the pattern inActorItemResolver.Performance and memory
The library gets polled many times a second, so per-poll allocations and redundant ReadProcessMemory calls add up fast. The headline change: GetActors used to deep-clone every tracked actor on every poll to build its removed-actor sets, then throw nearly all of those clones away. It now tracks present IDs in a HashSet and clones only the actors that are genuinely gone. GetGameState had a similar problem, running the full player-resolution pipeline every frame to answer a yes/no logged-in question that a single pointer read answers.
HashSet<uint>of present IDs populated during the address scan; clones are produced only for entries absent from those sets afterwards, so a steady-state poll allocates nothing for removal tracking.GetCurrentPlayer()pipeline inGetGameState()with an 8-byte CHARMAP pointer read. A non-zero pointer means logged in; a zero pointer falls back to the LoggedInStateLatch cache, preserving the existing zone-transition behavior without re-running actor resolution every frame.Math.Pow(distanceX, 2)withdistanceX * distanceX(and likewise for Y and Z) in bothGetDistanceToandGetHorizontalDistanceToinActorItemBase.cs, eliminating the transcendental-function overhead on the hot polling path.Math.Pow(expr, 2)calls withexpr * exprinCoordinate.Distance2D,Coordinate.DistanceTo, andCoordinate.Normalize, using localdouble dx/dy/dzvariables to avoid re-evaluating the subtraction expressions.[ThreadStatic] private static byte[] _singleByteBufferfield;GetBytelazily initializes it once per thread and reuses it, eliminating the per-callnew byte[1]allocation.GetStringto rent a buffer fromBufferPool, find the null terminator in-place, callEncoding.UTF8.GetString(bytes, 0, realSize)directly, and return the buffer in afinallyblock - eliminating both thenew byte[size]andArray.Resizeallocations.GetStringFromBytesto scan for the null terminator directly in thesourcearray atsource[offset + i], then callEncoding.UTF8.GetString(source, offset, realSize)- eliminating the intermediatenew byte[size]copy and theArray.Resizeallocation.if (!source.Any())withif (source.Length == 0)inGetStringFromBytes, eliminating the LINQ enumerator allocation on every call.GetStructure<T>body in atry/finallysoMarshal.FreeCoTaskMemis called unconditionally even ifPtrToStructurethrows, eliminating the native memory leak path.List<byte[]>allocated everyGetChatLogcall even when no new messages existList<byte[]>local with an instance field cleared at the start of each call, safe under the newGetChatLoglock, so steady-state polling reuses one list.!bytes.Any()withbytes.Length == 0in the foreach loop over bufferList, eliminating the LINQ enumerator allocation per chat buffer entry.new BgmSceneInfo[sceneCount]and up to 12new BgmSceneInfoobjects allocated every poll frameprivate readonly BgmSceneInfo[] _bgmSceneCache = new BgmSceneInfo[MaxBgmScenes]field; the scene loop now lazily creates BgmSceneInfo objects on first use and overwrites their fields in place each frame.GetByteArrayallocates a fresh 32-bytebyte[]on everyGetGameStatepoll frameprivate readonly byte[] _fftScratch = new byte[BgmAudibleBinCount * sizeof(float)]field on the Reader partial class inReader.GameState.csand switched the FFT read to use the pre-allocated scratch buffer via the voidGetByteArrayoverload, eliminating thenew byte[32]allocation on everyGetGameStatepoll..ExtractText()runs only when an id changes rather than on every poll.slotBytesallocated fresh on every container iteration - not returned to poolGetZoneNameperforms two uncachedGetSheet<T>dictionary lookups on every call_territorySheets,_placeNameSheets,_weatherSheets) soGetSheet<T>resolves once per language instead of on every call.Clone()when building removal candidates. Removed members are pruned from tracking before the result returns and the library never touches them again, and the mainPartyMemberscollection already hands callers live references, so the data callers see is unchanged.if (existing != null) { continue; }to immediately afterexistingis resolved - before theResolvePartyMemberFromBytescall.(uint mapID, uint mapIndex, uint mapTerritory) = this.GetMapInfo()intoGetTargetInfo()(called once per poll, before the four per-target calls) and passed the tuple as parameters toGetTargetActorItemFromSource(long, uint, uint, uint).= new XxxWorkerDelegate()field initializers from all five delegate fields (_chatLogWorkerDelegate,_monsterWorkerDelegate,_npcWorkerDelegate,_partyWorkerDelegate,_pcWorkerDelegate), keeping only the constructor assignments that were already there and that supply the instances actually used by the resolvers.FindExtendedSignaturesthat callsSignatureToByteonce per signature before entering any scan loop, storing results in a parallel array the scan loops index into, cutting conversions from signatures-times-windows to once per signature.AddOrUpdateupdate factory inAddHandlerto calloldHandler.Dispose()before returning the new handler, closing the leakedProcessHandleand stopping the orphaned background scan task when the same process ID is re-registered.entry.StatusItems.FirstOrDefault(x => x.CasterID == casterID && x.StatusID == statusID)with an index-basedforloop overentry.StatusItemsinActorItemResolver.cs.Regex.Replace(cleaned, @"[\r\n]+", ...)andRegex.Replace(cleaned, @"[\x00-\x1F]+", ...)calls inProcessNamewithNewLineRegex.Replace(...)andNoPrintingCharactersRegex.Replace(...), using the pre-compiled static fields already declared at the top of the class.ProcessFullLine: removedbytes.ToArray()(bytes is already byte[]) soEncoding.UTF8.GetStringreceives the array directly; replacedbytes.Count()withbytes.Length, eliminating the LINQ enumerator allocation per message.raw.Take(4).Reverse().ToArray()andraw.Skip(4).Take(2).Reverse().ToArray()with manually indexed byte arrays, replacedEncoding.UTF8.GetString(raw.ToArray())withEncoding.UTF8.GetString(raw), and replacedraw.Skip(8).ToArray()withBuffer.BlockCopy(raw, 8, cleanable, 0, cleanableLength), eliminating all four LINQ-chain and redundant-copy allocations per message.hex.AppendFormat($"{b:X2}")tohex.AppendFormat("{0:X2}", b), eliminating the per-byte interpolated string allocation insideByteArrayToString.EnsureArrayIndexes()call fromResolveEntriesand instead called it once inGetChatLog's else branch before anyResolveEntriesinvocations, so a ring-wrap (two consecutive ResolveEntries calls) no longer performs two redundant RPM round-trips of the 4000-byte index array.entry.Any()withentry.Length > 0inResolveEntries, removing the LINQ enumerator allocation per resolved chat entry.ResolveEntrynow callsthis._memoryHandler.GetByteArray(address, result)(the void overload) directly into the heap result array, eliminating the rented intermediate buffer and theBuffer.BlockCopy.entry.StatusItems.FirstOrDefault(x => x.CasterID == casterID && x.StatusID == statusID)with an index-basedforloop overentry.StatusItemsinPartyMemberResolver.cs.Concurrency and interop
IsAttachedwritten from the game-exit event thread without a memory barrier - polling thread may see stale valueIsAttachedauto-property with avolatilebacking field so writes from the process-exit event thread are immediately visible to the polling thread.this.Configuration.ProcessModel.Process.Handlewiththis.ProcessHandlein theReadProcessMemorycall insideGetStructure<T>, matching every other call site in the class._chatLogReader.PreviousArrayIndexand.PreviousOffsetmutated directly fromGetChatLog- two simultaneous calls produce interleaved cursor stateprivate readonly object _chatLogLock = new object()field and wrapped the entire post-guard body ofGetChatLoginlock (this._chatLogLock), preventing interleaved cursor state if two threads call simultaneously.[return: MarshalAs(UnmanagedType.Bool)]and changed the return type of theCloseHandleP/Invoke declaration frominttobool, matching the Win32 BOOL typedef and the established pattern used byReadProcessMemoryin the same file._loadingasprivate static volatile bool _loadingin all three lookup singletons (ActionLookup,StatusEffectLookup,ZoneLookup), ensuring the check-then-set sequence sees a current value across threads without the risk of a stale cached read on the checking thread._foundStatusesis a shared instance-levelList<StatusItem>cleared and mutated on every resolve call - unsafe ifResolveActorFromBytesis ever called concurrentlyprivate readonly List<StatusItem> _foundStatusesinstance field fromActorItemResolverand replaced it withList<StatusItem> foundStatuses = new List<StatusItem>()declared as a local variable insideResolveActorFromBytes._cache.GetOrAdd(sqpack, CreateGameData)may invokeCreateGameDataon multiple threads for the same key_cachefromConcurrentDictionary<string, GameData>toConcurrentDictionary<string, Lazy<GameData>>and updated bothGetOrNullandGetto useGetOrAdd(key, k => new Lazy<GameData>(() => CreateGameData(k))).Value, ensuring theCreateGameDatafactory is called at most once per sqpack path even under concurrent racing callers.Robustness
(pc ?? npc).Namewith the null-safe chainpc?.Name ?? npc?.Name ?? monster?.Name ?? string.Empty, producing the same empty-name result for unknown casters without throwing and catching an exception per entry.container.Amountread from foreign processprivate const int _inventoryMaxSlots = 600and a guardif (container.Amount > _inventoryMaxSlots) continue;inReader.Inventory.csbefore the slot-array allocation, preventing any oversized RPM or OOM from a torn read ofcontainer.Amount.Process.MainModule.FileNameaccess inTryResolveSqpackPathin a try/catch that returns null, soGetOrNullkeeps its never-throws contract when module info is unavailable.Task.Runbody intry/finallysoIsScanning = falseis always reached even when an exception escapes, and added a.ContinueWith(..., OnlyOnFaulted)continuation that logs the fault and routes it toRaiseException, consistent with the two analogousTask.Runcalls in theMemoryHandlerconstructor.Process game = candidates[0];, added aforloop that callscandidates[i].Dispose()for every index >= 1, releasing the OS process handles for any extra FFXIV instances that were returned byGetProcessesByNamebut not used.if (x + 2 >= bytes.Length) break;before thebytes[x + 2]read in case 2, andif (x + 4 >= bytes.Length) break;before thebytes[x + 4]read in the length==1 sub-path, so a truncated payload byte sequence breaks out of the switch cleanly instead of throwing.How this was audited
Eleven parallel reviewers swept disjoint parts of the codebase: eight by subsystem, three cross-cutting lenses for allocations, thread safety, and P/Invoke correctness. Every finding then went to an adversarial verifier that tried to refute it against the code; high-severity findings needed two independent confirmations to survive. A coverage pass at the end hunted for files and issue classes the sweep missed and contributed six of the confirmed findings. The full report with evidence and verifier notes for all 74 findings is in reports/audit-20260611.md, kept local since reports/ is gitignored.
🤖 Generated with Claude Code