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

fix overflow of web_order_line_item_id in 32-bit devices #384

Merged
merged 2 commits into from Nov 9, 2020

Conversation

aboedo
Copy link
Member

@aboedo aboedo commented Nov 6, 2020

Addresses #379

the web_order_line_item_id is actually a 64-bit integer, so parsing it would crash on 32-bit devices.

This PR forces the type to Int64, which will of course be slower but work correctly.

I believe there's another issue in 64-bit devices, because unrelated tests are not passing correctly for me on iPhone 5. I'll work on those separately.

@aboedo aboedo requested a review from vegaro November 6, 2020 21:20
@aboedo aboedo self-assigned this Nov 6, 2020
@aboedo aboedo added this to the 3.7.6 milestone Nov 6, 2020
@@ -6,12 +6,12 @@
import Foundation

extension ArraySlice where Element == UInt8 {
func toUInt() -> UInt {
func toUInt() -> UInt64 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't strictly necessary, but it doesn't do any harm other than parsing ever-so-slightly slower on 32-bit devices.
The alternative was to write essentially the same method twice, once with UInt and the other one with UInt64.

Comment on lines +23 to +26
func toInt64() -> Int64 {
return Int64(self.toUInt())
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is exactly the same as toInt on 64-bit devices.

Comment on lines +71 to +81
func int64Container(int64: Int64) -> ASN1Container {
let intAsBytes = int64ToBytes(int: int64)
let bytesUsedForLength = intAsBytes.count < 128 ? 1 : intToBytes(int: intAsBytes.count).count + 1

return ASN1Container(containerClass: .application,
containerIdentifier: .octetString,
encodingType: .primitive,
length: ASN1Length(value: intAsBytes.count, bytesUsedForLength: bytesUsedForLength),
internalPayload: ArraySlice(intAsBytes),
internalContainers: [])
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the same as intContainer, but with 64. Maybe it's because I'm really tired on a Friday, but I couldn't think of a much better way to do this avoiding duplication. Open to ideas.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for the other changes in this class

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine. It's more clear like this and they are not huge functions

@aboedo aboedo merged commit 0c7bdb8 into develop Nov 9, 2020
@aboedo aboedo deleted the fix/local_receipt_parsing_crash_on_32_bit_devices branch November 9, 2020 14:50
@aboedo aboedo mentioned this pull request Nov 11, 2020
@aboedo aboedo mentioned this pull request Jul 13, 2021
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.

None yet

2 participants