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

hex-literal: ensure '/' as last byte causes error in hex!() #665

Merged

Conversation

thomascastleman
Copy link
Contributor

Hello again,

I noticed that, because the way comment exclusion works, having a single forward slash / as the last byte given to hex!() causes no error to be given where there should be one. For instance, the following program compiles without error:

let _: [u8; 0] = hex!("/");

Note that this doesn't happen if there are bytes after the / (in which a case it errors).

My fix adds a check in the implementation of Iterator for ExcludingComments for if there are no more bytes and we're in the PotentialComment state. In this case it simply yields the previous byte (which was the foward slash) so that next_hex_val() will try to parse it as hex and panic with "invalid character". This has the nice property that it's consistent with the error you get currently if a single slash is encountered not at the end of the stream.

@newpavlov
Copy link
Member

Good catch! But can't we simply panic on PotentialComment without yielding the previous byte?

@thomascastleman
Copy link
Contributor Author

My thinking was to avoid replicating that same error message from next_hex_val, but if a panic right when we detect a final / is clearer, we can do that. How's this?

@newpavlov
Copy link
Member

Ah, ExcludingComments yields a single /, so it results in an "invalid character" error. I would change it a bit, but for now I think we can merge this PR as is.

@newpavlov newpavlov merged commit 04a1550 into RustCrypto:master Nov 11, 2021
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.

None yet

2 participants