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

Potentially Inconsistent Coordinate Slicing #299

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

dimitribouniol
Copy link
Contributor

@dimitribouniol dimitribouniol commented Nov 18, 2024

Replaced potentially unexpected accesses to coordinate slices with consistent ranges no matter if the raw representation is a root or a slice of data.

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

As discussed in slack, .prefix(upTo:) and .suffix(from:) are problematic when you don't know the origin of Data, as a slice of Data can have a non-zero startIndex, causing downstream misalignment. For myself, this happened when I tried accessing the rawRepresentation of an ECDSA's PublicKey, which was derived from the x963 representation, and thus had a start offset of 1. Although the code performed as expected, this seemed like a mistake waiting to happen as changing how rawRepresentation is derived for signatures could cause a similar headache in the future, especially so for anyone reading the code as a reference (it me!).

Modifications:

Since the use cases here were to split the Data in half, I replaced the uses of .prefix(upTo:) and .suffix(from:) with .prefix() and .suffix() respectfully.

Result:

No change to behavior, but more semantically correct code.

…nsistent ranges no matter if the raw representation is a root or a slice of data
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Nov 21, 2024
@Lukasa
Copy link
Contributor

Lukasa commented Nov 21, 2024

Thanks @dimitribouniol! ✨

@Lukasa Lukasa merged commit a3b7196 into apple:main Nov 21, 2024
32 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants