Skip to content

Mark Key::from_bytes as safe#236

Merged
jamesmunns merged 2 commits intojamesmunns:mainfrom
Veykril:veykril/push-uqrortlxzvzm
Jun 19, 2025
Merged

Mark Key::from_bytes as safe#236
jamesmunns merged 2 commits intojamesmunns:mainfrom
Veykril:veykril/push-uqrortlxzvzm

Conversation

@Veykril
Copy link
Copy Markdown
Contributor

@Veykril Veykril commented Jun 13, 2025

It doesn't seem to make sense to me as to why this is unsafe, when you have a safe constructor that takes an arbitrary string whose Key you could still use incorrectly. But also more importantly, Key implements Deserialize which means it is already possible to create abitrary keys via deserialization with arbitrary inputs.

So either this constructor ought to be safe, or Key::for_path needs to be also unsafe and the Deserialize impl has to be removed.

It doesn't seem to make sense to me as to why this is unsafe, when you have a safe constructor that takes an arbitrary string whose Key you could still use incorrectly.
But also more importantly, `Key` implements `Deserialize` which means it is already possible to create abitrary keys via deserialization with arbitrary inputs
@netlify
Copy link
Copy Markdown

netlify Bot commented Jun 13, 2025

Deploy Preview for cute-starship-2d9c9b canceled.

Name Link
🔨 Latest commit a1b66ae
🔍 Latest deploy log https://app.netlify.com/projects/cute-starship-2d9c9b/deploys/685431c4b30e31000847cd73

@Veykril Veykril changed the title Make Key::from_bytes as safe Mark Key::from_bytes as safe Jun 13, 2025
@jamesmunns
Copy link
Copy Markdown
Owner

Yeah, it being unsafe is probably a little strong. In general, I should make it more clear that Key should NEVER be used to guarantee type-safety, only as a hint to what type the frame may contain. Deserialization is still checked with serde, so you will never hit memory safety issues, but you could still get relatively "garbage" values out of the deser of the "wrong" type, same as any malicious packet.

I also should do a better job in postcard-rpc of enforcing that deser consumes all bytes, which will also help with "successfully deser'd the wrong type" issues.

Comment thread source/postcard-schema/src/key/mod.rs
@Veykril
Copy link
Copy Markdown
Contributor Author

Veykril commented Jun 13, 2025

Fwiw the mention of deserialize is because implementing Deserialize on a type effectively adds a new safe public constructor to the type. Whether your Deserialize impl is well behaved is not relevant for my argument.

@jamesmunns
Copy link
Copy Markdown
Owner

Yep, got it! Definitely aware of that.

@jamesmunns jamesmunns added the Q2 '25 Triage Items tracked as part of https://github.com/jamesmunns/postcard/issues/241 label Jun 19, 2025
@jamesmunns jamesmunns merged commit f4b28ff into jamesmunns:main Jun 19, 2025
5 checks passed
@jamesmunns
Copy link
Copy Markdown
Owner

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Q2 '25 Triage Items tracked as part of https://github.com/jamesmunns/postcard/issues/241

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants