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

Added the ability to specify a maximum depth for CBOR decoding #101

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

dimitribouniol
Copy link
Contributor

Added the ability to specify a maximum depth for CBOR decoding. This addresses a crash reported in #92 that can occur when certain types of data causes the recursive nature of the decoder to escape the stack for the given thread. I do suggest we set a more reasonable default than .max, though that can break some user's workflows if they weren't expecting it, so maybe requires a new version? Food for thought 😅

Copy link
Collaborator

@hamchapman hamchapman left a comment

Choose a reason for hiding this comment

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

Nice! I think what you've done here with exposing the option but setting the default value to Int.max is fine for now.

@hamchapman hamchapman merged commit 2b771dd into valpackett:master Mar 26, 2024
@dimitribouniol dimitribouniol deleted the dimitri/depth-limit branch March 26, 2024 21:35
@dimitribouniol
Copy link
Contributor Author

@hamchapman Could you cut a new release with this fix when you get a chance? Since crashes happened prior to this fix, I’d like to update the minimum version in https://github.com/swift-server/webauthn-swift and re-enable some fuzzing tests.

@hamchapman
Copy link
Collaborator

Yep, just tagged v0.4.7 and pushed a new version to Cocoapods

@hamchapman
Copy link
Collaborator

hamchapman commented Mar 27, 2024

We might have to tweak the new test you added given the CI failure here: https://github.com/valpackett/SwiftCBOR/actions/runs/8451024581/job/23148461601

I assume the CI runner is relatively low powered and so hits an issue quicker than a typical machine would

@dimitribouniol
Copy link
Contributor Author

Good catch! Let me make a new PR with a lower testing limit. It shouldn't impact code folks run though.

@dimitribouniol
Copy link
Contributor Author

@hamchapman Lowered it here: #103. Can you re-run the CI a few times before merging to see if 512 is low enough? I don't have access to that particular button unfortunately.

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.

2 participants