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

Convert bytes directly to Data instead of UnsafePointer API #83

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

jayrhynas
Copy link
Contributor

@jayrhynas jayrhynas commented Sep 20, 2024

Description

Replaces an incorrect use of UnsafePointer<UInt8>(array) to initialize Data.
There is already a Data(_ elements: Sequence<UInt8>) initializer we can use directly.

Motivation and Context

Using a pointer to an array created via the UnsafePointer initializer is undefined and isn't guaranteed to be valid. In particular, we ran into this issue when our RSA key started failing on iOS 18 release builds. The data in strippedKeyBytes was valid on the line above, but data was all zeroes after being initialized with UnsafePointer.

let strippedKeyBytes = [UInt8](byteArray[index...keyData.count - 1])
let data = Data(bytes: UnsafePointer<UInt8>(strippedKeyBytes), count: keyData.count - index)
return data

As stated in the "Important" note in the UnsafePointer documentation:

The pointer created through implicit bridging of an instance or of an array’s elements is only valid during the execution of the called function. Escaping the pointer to use after the execution of the function is undefined behavior. In particular, do not use implicit bridging when calling an UnsafePointer initializer.

How Has This Been Tested?

I ran the existing test suite in Release mode. Before the fix there were 16/32 passes, after the fix there are 32/32 passes.

This was tested with Xcode 16.0 (16A242d).
Devices:

  • iPhone 13 Pro running iOS 18.0 (22A3354)
  • MacBook Pro 14", 2021 running macOS 14.6.1 (23G93)

Using a pointer to an array created via the UnsafePointer initializer is
undefined and isn't guaranteed to be valid.
@CLAassistant
Copy link

CLAassistant commented Sep 20, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

@jayrhynas jayrhynas marked this pull request as ready for review September 20, 2024 02:13
Copy link

@yoller yoller left a comment

Choose a reason for hiding this comment

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

Good improvement. Replacing UnsafePointer with Data(strippedKeyBytes) enhances memory safety and avoids potential deallocation issues. This change also resolves the problem encountered on iOS 18, where using UnsafePointer caused the data to be zeroed out despite being valid in strippedKeyBytes.

Copy link

@euronovate-dev euronovate-dev left a comment

Choose a reason for hiding this comment

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

Tested that this change resolves the issue encountered on iOS 18

@Joebayld
Copy link

Joebayld commented Nov 8, 2024

This is the solution to #84

Can this get merged? The library currently does not work in Release mode on macOS 15.

@dannys42 dannys42 merged commit 4c9464b into Kitura:master Nov 14, 2024
2 checks passed
@dannys42
Copy link
Contributor

Thank you @jayrhynas for the fix and detailed description.

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.

6 participants