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

Decoding number string with too large exponent as JSON Integer leads to crash #812

Open
JCSooHwanCho opened this issue Aug 2, 2024 · 3 comments
Assignees

Comments

@JCSooHwanCho
Copy link

We encounterd our app crash on iOS 18.0 beta4.

for str in ["1147e0286", "1147e02864"] {
    do {
        let decoder = JSONDecoder()
        print(str, try decoder.decode(Int.self, from: str.data(using: .utf8)!))
        
    } catch {
        print(str, error)
    }
}

As an Investigation, It turns out that this crash is comes from precondition failure in JSONScanner.validateNumber(from: fullSource:). It determines too large exponent input as precondition failure. But in the same situation before iOS 18, It throws error gracefully and no crash occurred.

How about replace this precondition failure with regular error? It seems that there is appropriate error case in there(JSONError.numberIsNotRepresentableInSwift).

@JCSooHwanCho JCSooHwanCho changed the title decoding number string with too large exponent as JSON Integer leads to crash Decoding number string with too large exponent as JSON Integer leads to crash Aug 2, 2024
@kperryua
Copy link
Member

kperryua commented Aug 2, 2024

Thanks for the report. We've observed this crash internally as well. If you can provide any details on the impact this particular bug has your app, that would be helpful.

The way this particular code is designed, the preconditionFailure here is intended to catch mistakes earlier in the decoding of the number.

The initial JSON scanning phase does a quick, incomplete validation of the syntax of the number string with the intention of deferring full validation until the moment that the number is actually requested by the Decodable type (which it may never be, allowing us to elide unnecessary work).

When we call unwrapFixedWidthInteger() with a syntactically valid number region, the expectation is that the code prior to validateNumber will either successfully produce a number of the desired type, or it will throw an error indicating that the value cannot fit in the requested type. For a syntactically invalid number, no number is generated, and no "is not representable" error is thrown. The code then falls through to validateNumber() which is used to generate an error that describes the syntax error.

The intent is to avoid the work of fully checking the syntax of numbers that we know are already valid because they got parsed, but failed to convert to the desired integer type. However, I can also recognize that the code could be reconstructed to presume a final fallback condition of "numberIsNotRepresentableInSwift" after always re-validating syntax. (Performance in the error path is rarely a major concern, after all.)

The most recent code change here is what introduced this regression. We are exploring all possible fixes. Again, thank you for the report.

@JCSooHwanCho
Copy link
Author

JCSooHwanCho commented Aug 2, 2024

Thanks for the detail explanations. We've got some logic that trying decoding arbitrary string data.

  1. Try decoding as JSON.
  2. If failed, try string decoding.

But we encountered some problematic cases accidentally, so they lead our apps to crash.

I could find some workarounds, but I'm hoping to resolve this issue before the official release of new OSes.

@ncreated
Copy link

ncreated commented Sep 6, 2024

If you can provide any details on the impact this particular bug has your app, that would be helpful.

We are encountering the same issue in our SDK. Our situation involves using type-erasing AnyEncodable and AnyDecodable wrappers to persist user-defined Encodable attributes. Since we don't retain type information during encoding, we attempt to decode each value by trying a sequence of types, with Int being checked before Double.

We’re glad that this issue has been acknowledged. Is there a recommended mitigation or possible workaround we could implement in the meantime?

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

No branches or pull requests

3 participants