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

Correct comment on String::copy_into_slice #1055

Merged
merged 3 commits into from
Aug 25, 2023
Merged

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Aug 15, 2023

The copy routine here actually traps if you pass an output slice larger than the string. I think this is correct, since the previously-documented not-actually-performed behaviour (taking the minimum size of the output or the string) is a potential footgun / attack vector if someone tricks a contract into "copying N bytes" out of a string much smaller than N -- they get to basically force the output to be whatever the buffer's content was. The newly documented (already existing) behaviour does less magic / is less surprising / fails under more abnormal circumstances.

@graydon graydon requested a review from dmkozh August 15, 2023 23:43
@leighmcculloch
Copy link
Member

leighmcculloch commented Aug 22, 2023

the previously-documented not-actually-performed behaviour (taking the minimum size of the output or the string) is a potential footgun / attack vector if someone tricks a contract into "copying N bytes" out of a string much smaller than N -- they get to basically force the output to be whatever the buffer's content was.

I think it could be argued that copy_into_slice should fail unless the destination is exactly the same size. Right now it only fails if the source is smaller than the destination, but it silently works fine if it is larger than the destination. This silently working functionality could equally be a potential footgun / attack vector.

For example, this code will work without error:

let env = Env::default();
let expected = "my string";
let my_string = String::from_slice(&env, expected);

let mut short_array: [u8; 2] = [0; 2];
my_string.copy_into_slice(&mut short_array);

assert_eq!(short_array, "my".as_bytes());

(Code is taken from an example posted by @vinamogit on the stellar-dev Discord.)

@graydon
Copy link
Contributor Author

graydon commented Aug 24, 2023

@leighmcculloch I can see making that change on the specific API in the SDK you're describing; in the host interface it's actually more general already (it takes a start-position within the string, i.e. it allows not just doing a copy that's short from either the start or the end of the string). Would you want the env interface to be tightened also, requiring the user to do an explicit string-slice host function call before doing a copy-out call?

@leighmcculloch
Copy link
Member

I think it's reasonable to cover the footgun in the SDK without an env change.

@graydon
Copy link
Contributor Author

graydon commented Aug 24, 2023

@leighmcculloch ok I've updated it to panic on length mismatch, lmk if this is more to your liking.

@graydon graydon merged commit f9969b4 into main Aug 25, 2023
12 checks passed
@graydon graydon deleted the correct-string-comment branch August 25, 2023 02:16
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