-
Notifications
You must be signed in to change notification settings - Fork 119
Large integers cause overflows, especially on 32-bit hosts #76
Comments
Added extra option for arguments' sake. |
The latest RFC his this to say:
|
And this:
|
@mdmathias might be interested in that last comment. Ping. |
/me waves @jeremy-w into the thread |
Another option: saturate at min/max values rather than overflowing. We definitely don't want to take out the process; having some way to indicate there was a loss of range or precision in a field for those interested would be desirable, though. |
What would this look like on the user side? Swapping the type out on the fly doesn't seem to play well with Swift's type system. :\ |
Re: string swap, it'd be a thrown error during deserialization rather than parsing. But you could compensate for it in those places (move It's not my favorite solution, but it provides recoverability without loss of data. |
@zwaldowski re: RFC, do you think we should set and document the range and precision? And then throw an error in the parser if these are not respected? That does not feel unreasonable to me, given our options. |
I think it's the most sane path, but it also leaves the user (of the library) with an unrecoverable parse failure. |
I think the most important criteria from what you've mentioned are, in order, "without loss of data" and a distant second is "recoverability". That'd rule out |
TBH, I'm having trouble imagining when that |
They would have to change their model, yes, but that's at least a possible solution rather than a hard error. Obviously the best solution would be "tell the server to fix it" but as consultants we know we don't always have that luxury. |
So, we would still throw an error, but the error would contain a |
No, in the case I'm describing the |
👍 This would handle the common case of "I am treating your fooID as a cookie, and I don't care what type you think it should be". It would also let users default to String to start with where integer range might be a concern. |
Okay, so a client would do something like: let json = getSomeDataAndMakeItJSON()
let peopleJSON = try! json.array("people")
for personJSON in peopleJSON {
do {
let person = try Person(json: personJSON)
} catch JSON.Error.ValueNotConvertible(let theType) {
// do what here if the `Int` is too big?
}
} This feels like it would be hard to debug. Also, this doesn't feel all that recoverable. Perhaps I'm foggy-headed after the holiday party...but the model object won't be made, which is kind of a problem. I mean, we can prevent the crash with these errors, but the app won't have the data. In any case, the associated value on I'm sorry if I'm not contributing here; I'm just trying to wrap my head around the proposed solution. |
No, in your
Or any combination of that. If we do automatic Int->String conversion, then if you were aware of potential for overflow you would only use |
I see. I don't think that folks will intuitively default to writing that code. So, we'll have to provide better error information in my above example (which I think people will gravitate to more naturally) that will help folks get it write. That is, they will hit that |
I agree that it's not intuitive; this is not supposed to be a common case. I'm concerned adding the from-type to |
In my opinion, this is rare enough that we can start with option 2 (throw errors) and then revisit a workaround solution if people request it as a feature. |
@lyricsboy: I don't disagree, but I've already run into this on a project, so I was thinking about it in advance. It definitely is the route of least effort (in a good way). |
What was your resolution in that situation? |
Bourbon, I think. |
💥 |
From my perspective (missing a lot of the surrounding info that you guys have, of course), I'd prefer if it stayed as a number, or if Int64 was used throughout (I figure that was the topic of #57). That To me it makes more sense to keep a js/json number as a number for as long as possible, since ostensibly that's the type that the server intended the value to have. Although it is a good point that really large IDs and stuff might get messed around by this. |
I coded it up, and the path doesn't get hit in 64-bit processes. When I feed my 64-bit OS X build Swift.Int.max + 1 as the date value, CFNumber reads it in as a |
Trying to run the MobileFreddy tests on iPhone 5, and I get a "run destination not compatible" error. #ThanksXcode |
Seeing the same here. I figure NSJSONSerialization is putting the value through as an Int64, but Freddy's trying to use an Int32, so the value overflows. I think this would affect any value in (Int32.max...Int64.max) on a 32-bit device |
Oops, I fed it UINT64_MAX+1 earlier. But INT64_MAX+1 on a 64-bit machine still results in a double. As a workaround, pulling out the value using the |
- Freddy currently has an issue parsing large integers on 32 bit devices. See discussion: bignerdranch/Freddy#76.
I noticed the issue when I test my app on 32bit device, all JSON parsing will just fail. I expected the big integer issue and workaround it by accessing the string version of the field, but Freddy will always fail even I didn't touch the big integer field. let str = "{\"id\": 598458199574970368, \"id_str\": \"598458199574970368\"}"
let data = str.dataUsingEncoding(NSUTF8StringEncoding)!
let json = try? JSON(data: data)
XCTAssertEqual(try json!.string("id_str"), "598458199574970368")
IMHO the "parse as string" workaround is not necessary, if people need big integer in JSON, they probably using string value already. A Integer value is just not a String value. |
@jeremy-w thanks for the pointer! I like Freddy, and I'll have to support parsing file with big integer, this definitely give me best of both worlds. |
You're right – that isn't working as a workaround because of the JSON building. It avoids the crash, but mangles the value:
That's ETA: If your JSON has |
unfortunately I do not have control over that JSON format, I guess I could do some temporary hackery to force all the date formats to float before handing them off to the parser for now or try to fix this locally. |
I've brought the 32-bit issue up on #152, so we should be able to fix this along with the other overflow issues. |
Due to this issue, In our project, we have to switch from Freddy to SwiftyJSON. Regrettably, you don't have |
Freddy turns
|
NSNumber is not a bignum implementation, so the problem reappears on 64-bit platforms once you overflow the 64-bit range, and NSNumber still does not resolve the issue. It would add another layer of wrapping and clumsiness, but not solve the underlying issue. |
Gotcha. So NSNumber would solve the problem of using Freddy to serialize/deserialize Int64s on a on a 32 bit platform but not serializing/deserializing integers larger than 64 bits on a 64 bit platform. Could you just explicitly add support for Int64? Or Int64 and Int32? |
We haven't seen a way to add support for the sized integers without making the common case ( |
Follow-on from #57. Consensus was that
JSON.Int
should continue to useSwift.Int
as its associated type, but that still leaves the issue of overflows at hand. The stdlib asserts inside the parser, generally bringing down the whole app as a result.Options include:
&+
et. al. operators and explicitly allow overflows.addWithOverflow(_:_:)
et. al. functions, catch overflows, and throw errors, thus cancelling the parse.addWithOverflow(_:_:)
et. al. functions, catch overflows, and returnString
s for big integers. This might require some workflow changes in the parser.addWithOverflow(_:_:)
et. al. functions, catch overflows, and returnDouble
s for big integers. (undesirable, but I mention it)Workaround
If this is a problem for you today, you can work around it by using Apple's parser rather than Freddy's via
JSON(data:usingParser:)
and specifying the parser asNSJSONSerialization.self
.For a bit more discussion around this, see this comment below.
Discussion Summary
Eventual decision was to avoid overflow by treating numbers that would overflow as strings. The value can be pulled out and then parsed from the string if needed by the user of the API. Implemented in #152 and expected to land in Freddy v2.1.
The text was updated successfully, but these errors were encountered: