Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide codingPath when decoding fails due to corrupt data #10

Merged

Conversation

mrackwitz
Copy link
Contributor

🎯 Motivation

When a data corruption error occurs, it's currently hard to tell what was responsible for the corruption. Beside indeed corrupted data, corruptions can be caused by mismatching Encodable and Decodable implementations, as well as internal errors of this library around the coding and decoding process.

Codable allows to maintain codingPaths, which is already partially implemented. However it is not fully utilized yet to indicate where an errors occurs throughout the decoding process.

This PR makes sure that all errors due to data corruption are annotated with the coding path of the currently decoded key. As all errors thrown are of the case DecodingError.dataCorrupted, it is sufficient to handle just said errors.

💪 How

This PR adds a private helper method wrapError(forKey: _, _) to KeyedContainer, which catches the relevant case of DecodingError and composes the path, based of the containers coding path, the given key and coding path already set on the error's context. This is done when decoding nested values (via decode(_, forKey: _)) and retrieving nested containers.
Note some limitations:

  1. The KeyedContainer of BinaryCodable is eagerly reading all keys on initialization. If a container's data is syntactically invalid (e.g. keys or values of invalid length), then this will fail early. This is handled by additional error handling logic in the KeyedContainer's initializer, but if a data corruption causes a byte length difference within a nested key or value, this cannot be indicated and is likely to cause an error at the next sibling or an ancestor in the tree.
  2. The KeyedContainer does not include yet the super key on decoding errors within the coding path.

In similar fashion, this PR also utilizes now the private helper method wrapError(_) in UnkeyedContainer. This was previously unused. Further it does update said method's implementation, so that it does attribute the error to a coding path which includes the currentIndex. To achieve that, I've added a helper struct AnyCodingKey to the library, which is essentially a type-erased CodingKey allowing to represent arbitrary coding keys values without a concrete type.

🧪 Testing

To ensure the correctness of the added error handling, I've added a relatively comprehensive test case CodingPathTests. I have chosen a white box testing approach here, coming up with a series of tests cases which should ensure to hit all relevant code paths. I've done this by relying only on the public API by testing through decoding crafted corrupted data.

To achieve this, all tests:

  1. Correctly encode their values with the data structure they define.
  2. Programmatically mutate the binary representation.
  3. Validate whether the so corrupted bytes do equal with the expected and documented binary representation.

This ensures that the tests stay maintainable, by being self-documenting and preventing that changes to the utilized data structure lead to cryptic failures, because the crafted corrupted data mismatches.

Within the tests, I've relied on some helpers structs:

  1. NestedBox<T>: This is consistently used as the root value of all encoded values to ensure that there is at least one coding key val inherited throughout the coding path. This enables the tests to validate that this inheritance is correctly taking place.
  2. CorruptBool: This type decodes a boolean val. If it is true, then it decodes without errors, but it throws a data corruption error when attempting to decode a false value. Once again, this is encoding/decoding to a keyed nested container to test that the presence of the coding key within the coding path.

Most tests rely on a single byte being removed or 1<>0 bit flips. While some of these tests might even cover useful real-world scenarios with correctly working encoding and decoding implementations, this is not given for all tests. Particularly when bytes were removed, some tests are also making sure to account for this in the encoded byte lengths to ensure that the error is only thrown for a coding path further nested in the tree. Such errors are obviously extremely unlikely to occur in the wild by chance. But this seems useful in development, when encoding and decoding implementations are likely to be not correctly aligned.

@christophhagen
Copy link
Owner

This is an improvement, thanks for that.
It's a good idea to provide as much context for errors as possible, although we have to remember that when it comes to data corruption, all bets are off. Arbitrary changes to the binary can lead to arbitrary errors, and the provided context may become misleading. But at least in case of invalid custom encoding/decoding logic, this may be very helpful.

@christophhagen christophhagen merged commit f7663e6 into christophhagen:master Jan 15, 2024
2 checks passed
@mrackwitz mrackwitz deleted the coding-path-on-decoding branch January 15, 2024 19:11
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.

2 participants