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

Replace SwiftCBOR with PotentCodable #36

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

marius-se
Copy link
Collaborator

There is a bug in SwiftCBOR causing an application to crash when decoding large amounts of random (/invalid CBOR) bytes. I tried to find the source of this problem in SwiftCBOR, but wasn't successful with that unfortunately.

@dimitribouniol
Copy link
Collaborator

For what it's worth, if we want to still use SwiftCBOR, I submitted a fix here valpackett/SwiftCBOR#101 that we can use as a subclass to CBORDecoder if they don't merge the fix.

@dimitribouniol
Copy link
Collaborator

dimitribouniol commented Feb 15, 2024

that we can use as a subclass to CBORDecoder if they don't merge the fix

nvm, it seems like CBORDecoder is not open, so it can't be subclassed 🫠

@marius-se
Copy link
Collaborator Author

Ah nice! I didn't mention it in this PR yet but PotentCodable has the exact same issue :D
I'd still prefer to migrate to PotentCodable since it seems to be more actively maintained. @dimitribouniol would you be okay with shipping the same fix to PotentCodable? I created an issue a few weeks ago here: outfoxx/PotentCodables#65

@dimitribouniol
Copy link
Collaborator

Yeah, I saw it afterwards haha. It should be possible, though it'll be a bit more involved since PotentCodables uses a struct for reading, so we'll need to pass the depth down to everything which is a bit more error prone.

Should we consider instead forking SwiftCBOR, cleaning it up with documentation and additional niceties (OrderedDictionaries for one), and making it available under the swift-server umbrella? Their license permits this, though I wanted to check first here to see if there was interest and to also ask Val via Mastodon or something if they were alright with it.

@0xTim
Copy link
Member

0xTim commented Mar 20, 2024

It should probably live in the swift-server-community org but I'd prefer to see the changes upstreamed if the maintainer is happy to accept a PR

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.

3 participants