[Foundation] Fix NSCoder.Encode(byte[], offset, count, key) to honor offset/count#26015
[Foundation] Fix NSCoder.Encode(byte[], offset, count, key) to honor offset/count#26015rolfbjarne wants to merge 2 commits into
Conversation
…offset/count. The offset/count overload validated its arguments but ignored them: it pinned the start of the buffer and passed buffer.Length to EncodeBlock, so it always encoded the entire array instead of the requested [offset, offset+count) segment. Pin the buffer and pass (IntPtr)(p + offset) and count instead, matching the behavior of the sibling Encode overloads. Also fix the existing CoderTest to assert the encoded segment is exactly the [offset, count) slice (the previous assertion encoded the buggy whole-buffer behavior). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a correctness bug in the Foundation NSCoder.Encode (byte[], offset, count, key) overload where offset/count were validated but not actually honored, and updates the corresponding regression test to assert slice-based behavior instead of the previous (buggy) full-buffer round-trip.
Changes:
- Update
NSCoder.Encode (byte[], offset, count, key)to passp + offsetandcounttoEncodeBlock. - Adjust
CoderTest.EncodeDecodeTestto assert that decoding"buffer2"returns only the requested segment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Foundation/NSCoder.cs | Fixes encoding logic to honor offset/count when encoding byte segments. |
| tests/monotouch-test/Foundation/CoderTest.cs | Updates assertions so the test validates slice encoding rather than full-buffer encoding. |
Address review feedback: with offset=2,count=1 the count happened to equal buffer.Length - offset, so an implementation that encoded buffer.Length - offset bytes (instead of count) would still pass. Using offset=1,count=1 keeps count independent of buffer.Length - offset and offset non-zero, so offset and count are both exercised. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🔥 [CI Build #68b6437] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 1 tests failed, 198 tests passed. Failures❌ monotouch tests (MacCatalyst) [attempt 2]1 tests failed, 16 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ assembly-processing: All 1 tests passed. [attempt 2] Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Why
NSCoder.Encode (byte[] buffer, int offset, int count, string key)validated itsoffsetandcountarguments but then ignored them. It pinned the start of the buffer and passedbuffer.LengthtoEncodeBlock, so it always encoded the entire array regardless of the requested[offset, offset+count)segment. This is a latent correctness bug: callers asking to encode a slice silently got the whole buffer.The bug was found during the issue #3590 Span-API survey. A future
ReadOnlySpan<byte>overload would fix it implicitly, but the existing overload deserves its own targeted fix and test now.What
src/Foundation/NSCoder.cs: pin the buffer and callEncodeBlock ((IntPtr) (p + offset), count, key), matching the pointer idiom used by the siblingEncodeoverloads.tests/monotouch-test/Foundation/CoderTest.cs: the existing test already encoded{3,14,15}withoffset 2, count 1under key"buffer2", but its assertion checked that the whole buffer round-tripped, i.e. it asserted the buggy behavior. Updated it to assert the decoded segment is exactly the[offset, count)slice (buf.Length == 1andbuf[0] == buffer[2]). This fails with the old code and passes with the fix.Testing
Microsoft.iOS.dllby disassembling it: the IL now computesp + offsetand passescounttoEncodeBlock.🤖 Pull request created by Copilot