-
Notifications
You must be signed in to change notification settings - Fork 95
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
Fix stack overflow when decoding big array newtypes wrapped in a Box
#462
Fix stack overflow when decoding big array newtypes wrapped in a Box
#462
Conversation
derive/src/decode.rs
Outdated
|
||
Some(quote!{ | ||
// Just a sanity check. This should always be true and be optimized-out. | ||
assert_eq!(::core::mem::size_of::<Self>(), #(#sizes)*); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is really needed here. Would you please explain why you've put it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is not needed per-se; it's just a defense-in-depth sanity check to make sure the assumptions hold. And because I really like sprinkling assertions around for things that should always hold true. (:
(Normally I'd do this kind of an assert as a static assert, but this being a derive macro makes it very tricky due to generics.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this reminds me - we also need one more assert here to be comprehensive (an assert which checks that there's only one non ZST field); let me add that in real quick.
And to further explain why it's nice to have these: the #[repr(transparent)]
should pretty much guarantee that this is correct, but if for some reason we'd have a bug where the check for #[repr(transparent)]
doesn't work then the asserts would trigger and the whole thing would fail without being unsound (which we must avoid at all costs).
This is not just a theoretical scenario; this has actually happened in my own serialization crate! When I upgraded to syn 2.x the new version of syn had changed the way how it exposes attributes in a backwards-incompatible way, and my check for #[repr(transparent)]
started to silently fail (well, not really because I had tests, but you do get the point).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, makes a lot of sense. I like this defensive approach, especially when #[repr(transparent)]
could be by-passed :), did not think of this.
for field in fields { | ||
let field_type = &field.ty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe error if the length is not 1
to actually ensure that the assumption above holds?
Then there is also no loop needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a valid use case to have e.g. a PhantomData
in your newtype to hold a generic T
, so I didn't really want to limit this.
Okay, added an extra assert to also make sure the number of non-ZST fields is correct, and added more tests. (Now the tests are guaranteed to fail if the check for |
Followup of #426
This PR fixes decoding of big array newtypes wrapped in a
Box
, e.g. previously only this was correctly deserialized:And now this will also be correctly handled:
(The newtype must be marked with
#[repr(transparent)]
; without that attribute it will still crash.)Fixes #419