sysvar: Fix UB in sysvar loading

[jamie-osec]

#257 (comment)

Slices must be initialized, but the implementation of Sysvar::get creates a &mut [u8] from an uninitialized buffer. This causes UB and miscompiles.
Resolve this by introducing a new private function which takes a mutable pointer (may be uninitialized) and use that instead.

[github-actions]

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

[rustopian]

Good find, and thanks for moving on it quickly @jamie-osec! This is similar to an auditor-noted issue in p-token (since remedied), and it should be fixed.

Your proposed fix isn't quite there yet. I'm away from my machine, but for now I've approved the CI so that you can easily see its objections.

[github-actions]

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

[febo]

Good catch! Just left a couple of nits.

[jamie-osec]

I've been investigating this further to confirm the issue and I've realized the problem is actually that get_sysvar returns the raw sysvar buffer data. In the case of Rent (and other sysvars), this data is stored as bincode-serialized data, whereas the old implementation deserialized the data with the desired type.
This means that:

• There is a mismatch between the length param the program passes (i.e. 24 bytes) and the sysvar buffer data (17 bytes)
• This causes the runtime to return OFFSET_LENGTH_EXCEEDS_SYSVAR-
• Which get_sysvar maps to UnsupportedSysvar
• Which manifests as the error The arguments provided to a program instruction were invalid.

While the original issue is library-level UB according to the documentation, it does not appear to be currently considered UB by Rust, and Miri does not report an issue.

[github-actions]

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

[github-actions]

If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client @solana/sysvars (example)
Thank you for keeping the JavaScript clients in sync with the Rust clients.

[jamie-osec]

Addressed the UB nits, and pushed a fix for the root cause.

• Added a serialized_size param to the macro, and test that the size is indeed correct for the serialized data
• Adjusted the to_bytes function to serialize rather than using raw bytes to match the runtime
• Fallback to the original sysvars when bincode is not enabled

I'm not familiar enough with the context to know if this breakage was intended - it seems unlikely given that the signature is still returning Self which in many cases is not possible without deserialization. If it is, I can revert back to just the UB fixes.

(Relevant: #87)

[rustopian]

While the original issue is library-level UB according to the documentation, it does not appear to be currently considered UB by Rust, and Miri does not report

Indeed, this is why it didn't raise red flags earlier, but it's still a good idea to fix it, as it violates the contract of from_raw_parts_mut and could in the future cause issues.

[rustopian]

Fallback to the original sysvars when bincode is not enabled

Thanks for tracking down the issue!

FYI, we likely want to use the general syscall when possible due to #235.

[febo]

Fallback to the original sysvars when bincode is not enabled

Thanks for tracking down the issue!

FYI, we likely want to use the general syscall when possible due to #235.

Using bincode on-chain will most likely offset any gains from sol_get_sysvar. An alternative is to create 1-byte aligned sysvar representations so we can zero-copy the data; or keep using sol_get_<NAME>_sysvar.

[rustopian]

Using bincode on-chain will most likely offset any gains from sol_get_sysvar. An alternative is to create 1-byte aligned sysvar representations so we can zero-copy the data; or keep using sol_get_<NAME>_sysvar.

Yes, I'm definitely not suggesting we require/encourage bincode.

[rustopian]

@jamie-osec, thanks much for your work here. Due to the need to fix the 3.1.0 crate in the interim, I'm going to close this PR and just revert to the old (3.0.0) sysvar behavior for now. Then, we'll ensure everything works properly and cleanly implement an update. There are clearly some test coverage improvements needed.

Your UB fix will be incorporated and I'll make sure you receive attribution for it. I'll tag you on the new PR once open.

[jamie-osec]

Great, thanks for the update