Skip to content

fix: bound zip archiver standard error collection - #324

Open
KevinWang-wpq wants to merge 2 commits into
Squirrel:mainfrom
KevinWang-wpq:fix/bound-zip-archiver-stderr
Open

fix: bound zip archiver standard error collection#324
KevinWang-wpq wants to merge 2 commits into
Squirrel:mainfrom
KevinWang-wpq:fix/bound-zip-archiver-stderr

Conversation

@KevinWang-wpq

@KevinWang-wpq KevinWang-wpq commented Jul 21, 2026

Copy link
Copy Markdown

Summary

  • Replace unbounded RACSignal collect stderr accumulation with a streaming
    aggregate that retains at most 1 MiB per ditto invocation.
  • Stop the readability handler after EOF, avoiding unnecessary empty-pipe work.
  • Add a regression test that makes a failing task emit 2 MiB on stderr and
    verifies that the retained error text is capped.

Root cause

SQRLZipArchiver forwards every NSFileHandle.availableData callback into a
RACSubject. collect retains every chunk in an NSMutableArray, after which
the code concatenates all chunks into another NSMutableData. A malformed
archive can make ditto write excessive diagnostics and cause an unbounded
allocation in the host process.

Observed crash path (root-cause-relevant frames)

[Chromium / PartitionAlloc
 base/allocator/partition_allocator]
partition_alloc::internal::OnNoMemoryInternal
partition_alloc::internal::TerminateBecauseOutOfMemory
partition_alloc::internal::OnNoMemory
partition_alloc::internal::PartitionExcessiveAllocationSize
partition_alloc::internal::PartitionDirectMap

[Chromium / allocator shim]
allocator shim allocation path

[Apple Foundation]
-[__NSArrayM insertObject:atIndex:]

[ReactiveObjC
 ReactiveObjC/RACSignal+Operations.m]
-[RACSignal collect]

[Squirrel.Mac
 Squirrel/SQRLZipArchiver.m]
__23-[SQRLZipArchiver init]_block_invoke.44

[Apple Foundation]
-[NSConcreteFileHandle _monitor]

PartitionExcessiveAllocationSize is a deliberate PartitionAlloc termination
when one allocation exceeds the direct-map limit (roughly 2 GiB), rather than a
generic low-memory termination. Here the unbounded collect array is the
allocation owner; the Foundation frame is where that array growth reaches the
allocator.

The error output is used only to populate NSLocalizedDescription when the
ditto task exits unsuccessfully. Exit status and normal archive behavior are
unchanged. The first 1 MiB of diagnostics is retained; later data is still read
and discarded so the child process cannot block on a full stderr pipe.

@KevinWang-wpq
KevinWang-wpq marked this pull request as ready for review July 21, 2026 12:12
@KevinWang-wpq

Copy link
Copy Markdown
Author

Hi @MarshallOfSound — this bounds unbounded stderr collection in SQRLZipArchiver and adds a regression test for excessive diagnostic output. Since you recently updated this file in #308, could you please take a look when you have a chance?

@MarshallOfSound MarshallOfSound left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified locally, SQRLZipArchiverSpec all green including the new test. Nice — factory variant of aggregate is the right call given the repeat, and killing the readabilityHandler on EOF fixes the post-EOF spin as a bonus. Few things inline.

Comment thread Squirrel/SQRLZipArchiver.m Outdated
Comment thread Squirrel/SQRLZipArchiver.m Outdated
Comment thread Squirrel/SQRLZipArchiver.m
Comment thread SquirrelTests/SQRLZipArchiverSpec.m
@KevinWang-wpq

Copy link
Copy Markdown
Author

@MarshallOfSound Thanks again for the review — I have addressed all of the inline suggestions in commit 0a8ae53.

  • aligned the constant naming with the repository convention;
  • removed the unreachable empty-data guard;
  • made truncation preserve a valid UTF-8 boundary and stop accepting subsequent chunks once capped;
  • strengthened the regression test with a UTF-8 boundary case and a lower-bound assertion.

script/test -derivedDataPath DerivedData passes (88 tests). Could you please take another look when you have a chance?

@KevinWang-wpq

Copy link
Copy Markdown
Author

@MarshallOfSound I wanted to get your take on a simpler alternative for the UTF-8 handling before changing the current implementation.

Instead of finding a UTF-8 boundary while accumulating stderr, we could keep the original bounded NSMutableData accumulator unchanged. At the final decode point, attempt UTF-8 decoding normally; only if it returns nil, retry after removing 1, then 2, then 3 bytes from the end.

Because the cap only slices the tail of the byte stream, any new invalid UTF-8 introduced by the cap should be confined to that suffix; a UTF-8 scalar is at most 4 bytes. This keeps the 1 MiB cap behavior unchanged and avoids the byte-mask helper plus the extra truncated state in the accumulator. The retry would only run once on the task-failure path.

I realize your inline suggestion preferred boundary handling over a decode-retry loop, so I have not changed the implementation to this approach. Would you be open to this tradeoff, or would you prefer to keep the current boundary-at-truncation approach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants