Changes to NSKeyedArchiver plist decoder to decode object references once #5325

[elhoim]

BLUF

• Priority: medium
• Problem: NSKeyedArchiverDecoder re-decoded a shared object every time it was referenced; a plist whose references fan out onto the same shared subtree expanded into an exponential number of decode operations even for a tiny object table.
• Fix: memoize decoded objects by their plist UID so each shared object is decoded once and reused; the existing path-based cycle check is kept and path-dependent (cyclic) subtrees are left uncached, so decoded values are unchanged.
• Test: a diamond fan-out regression test asserts decoding stays linear in the object table, plus a mutually-referencing structure test that pins the exact decoded value.
• Verified: probe, 23 existing tests, real NSKeyedArchiver.plist, and cyclic-structure output all confirm identical results with a linear decode count.

Details

NSKeyedArchiverDecoder.Decode walks object references (CF$UID values) into
the $objects array. Previously each reference was decoded on its own via
_DecodeObject(objects_array[uid], ...) with only a path-based cycle check and
no reuse of already-decoded objects. A reference fan-out onto one shared subtree
therefore re-decoded the same objects repeatedly, which grows exponentially:

levels=10 objects=13 _DecodeObject calls=        2,047 elapsed=0.004s
levels=14 objects=17 _DecodeObject calls=       32,767 elapsed=0.064s
levels=18 objects=21 _DecodeObject calls=      524,287 elapsed=1.068s
levels=20 objects=23 _DecodeObject calls=    2,097,151 elapsed=4.589s

The call count doubles with every added level while the object table stays tiny.

The fix adds a per-plist cache keyed by plist UID. A new
_DecodeReferencedObject helper returns the cached value on a repeat reference
and otherwise decodes and stores it. To keep behaviour identical for objects
that reference each other, a frame is only cached when no cycle reference was
skipped anywhere in its subtree (tracked with a running _cycle_skip_count);
any object on a cycle is fully traversed on its first visit, hits a
back-reference, and is therefore not cached, so its path-dependent value is
preserved. The cache is reset at the start of each Decode call and cleared
afterwards, because a decoder instance is reused across plists
(macos_background_items, sqlite_plugins/ios_instagram).

References to the same UID now yield the same Python object rather than a fresh
copy. Decoded plist values are consumed read-only in plaso/parsers/plist_plugins/
and ios_instagram (grepped: no pop/del/in-place mutation of decoded values),
so sharing is safe; equality of results is unchanged.

Verified:

• Probe above: 20 levels / 23 objects → 2,097,151 _DecodeObject calls (~4.6 s)
  on main, doubling per level; after the fix → 21 calls.
• 23 tests pass across tests/parsers/plist_plugins/interface,
  macos_background_items, sqlite_plugins/ios_instagram, parsers/plist.
• test_data/plist/NSKeyedArchiver.plist decodes to an equal value before and
  after.
• The mutually-referencing structure decodes to the same
  {'a': {'b': {}}, 'b': {'a': {}}} before and after.
• Gates on changed files: black --check, pylint --rcfile=.pylintrc, typos.

Fixes #5325

[codecov]

Codecov Report

❌ Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.57%. Comparing base (323c096) to head (5ac3375).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
plaso/parsers/plist_plugins/interface.py	88.23%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5326      +/-   ##
==========================================
+ Coverage   85.54%   85.57%   +0.02%     
==========================================
  Files         458      458              
  Lines       41563    41633      +70     
==========================================
+ Hits        35555    35626      +71     
+ Misses       6008     6007       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[joachimmetz]

Change "_cycle_skip_count" into a name that closer matches it purpose

[joachimmetz]

please solve this without mock

[elhoim]

Addressed both review points in 5ac3375:

• Renamed _cycle_skip_count to _number_of_cyclic_references. It counts references skipped because they point back to an object already on the current decode path, and that running count is what decides whether a decoded subtree is path-dependent and must therefore not be reused, so the new name matches its purpose.
• Reworked testDecodeWithSharedReferences to prove shared-reference decoding without mock and without any timing assertion: it now asserts that each reference to a shared child resolves to the same decoded object and that decoding terminates with the correct values. The cyclic-structure test was already mock-free and is unchanged.

The decode-once property is stated in the PR description.