fix: CBOR subscript force unwrap that crashes on nil assignment#126
Open
hamchapman wants to merge 5 commits into
Open
fix: CBOR subscript force unwrap that crashes on nil assignment#126hamchapman wants to merge 5 commits into
nil assignment#126hamchapman wants to merge 5 commits into
Conversation
These changes address multiple critical safety issues: **Issue #118 - Large allocation crash:** - Replace `.map()` with progressive array building in `readN()` - Prevents memory allocation crashes from malformed CBOR with huge declared lengths - Add capacity reservation capped at reasonable size (1024) **Issue #69 - Array bounds checking:** - Add bounds validation before accessing data ranges in `UnkeyedDecodingContainer` - Prevents crashes from corrupted data with out-of-bounds ranges - Throw proper `DecodingError` instead of crashing **Unsafe `try!` replacements:** - AnyCodingKey: Replace `try!` with proper error handling for invalid key types - KeyedDecodingContainer: Replace `try!` with proper error propagation - KeyedDecodingContainer: Replace `fatalError()` with throwing DecodingError - UnkeyedDecodingContainer: Remove `fatalError()`, allow errors to propagate - AnyCodingKey.encode(): Replace `fatalError()` with throwing EncodingError - AnyCodingKey.key(): Replace `fatalError()` with preconditionFailure (better messages) **Encoder changes:** - Make `encodeMap()` and `encodeMapChunk()` throw instead of using `try!` - Update call sites with proper error handling - Add `preconditionFailure` for programming errors (`forbidNonStringMapKeys` violations)
Previously, `superEncoder()` and `superEncoder(forKey:)` methods in both
`KeyedEncodingContainer` and `UnkeyedEncodingContainer` would crash with
`fatalError("Unimplemented")`.
This commit implements these methods to properly support Swift class inheritance
with Codable, allowing subclasses to encode their superclass properties into
nested containers.
**Changes:**
- `KeyedEncodingContainer.superEncoder()`: Creates encoder under "super" key
- `KeyedEncodingContainer.superEncoder(forKey:)`: Creates encoder under custom key
- `UnkeyedEncodingContainer.superEncoder()`: Appends encoder as array element
- Make `_CBOREncoder` conform to `CBOREncodingContainer` protocol
This change includes 2 main changes. Previously, the `maximumDepth` option was defined but not actually used in `CodableCBORDecoder`, creating a DoS vulnerability where deeply nested structures could cause stack overflow. **Problem:** 1. `CodableCBORDecoder` had a `maximumDepth` property but it wasn't included in the options computed property, so it defaulted to `.max` 2. `SingleValueDecodingContainer` called `CBOR.decode()` without passing options 3. `KeyedDecodingContainer` and `UnkeyedDecodingContainer` created `CBORDecoder` instances without passing options 4. This meant depth checking was never enforced for Codable decoding **Fix:** - Add `maximumDepth` property to `CodableCBORDecoder` (public API) - Include `maximumDepth` in options computed property - Update `setOptions()` to set `maximumDepth` - Pass options to all `CBOR.decode()` calls in `SingleValueDecodingContainer` - Pass options to `CBORDecoder()` calls in `KeyedDecodingContainer` - Pass options to `CBORDecoder()` calls in `UnkeyedDecodingContainer` Changes: - Added `currentDepth` field to `_CBORDecoder` and all container classes (`KeyedContainer`, `UnkeyedContainer`, `SingleValueContainer`) - Thread `currentDepth` through all container creation and nested decoding - Check depth before creating keyed/unkeyed containers in `_CBORDecoder` - Increment depth when creating nested decoders in `decode()` methods - Pass incremented depth through `superDecoder()` methods Previously, each `CBOR.decode()` call would reset `currentDepth` to 0, allowing deeply nested structures to bypass the depth limit. For example, a 10-level nested array with `maximumDepth=5` would incorrectly succeed. Now depth is properly tracked across the entire container hierarchy.
…ortMapKeys` The parameter name in `CBOROptions.init()` was incorrectly spelled as `shouldShortMapKeys` when it should be `shouldSortMapKeys` to match the property name and convey the correct meaning (sorting keys, not shortening them).
The subscript setter had force unwraps that would crash when setting a value to `nil`. Now we don't force unwrap and when setting an array's value by index to `nil` the element will be updated to become a `CBOR.null`.
hamchapman
force-pushed
the
hc/fix-typo-in-shouldsortmapkeys-option
branch
from
e4cbb36 to
d572f48
Compare
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
1 participant
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.
The subscript setter had force unwraps that would crash when setting a value to
nil.Now we don't force unwrap and when setting an array's value by index to
nilthe element will be updated to become aCBOR.null.