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

[bug]: Donot use assert! in NationalNumber::new function #69

Closed
12089897411 opened this issue Apr 16, 2024 · 6 comments
Closed

[bug]: Donot use assert! in NationalNumber::new function #69

12089897411 opened this issue Apr 16, 2024 · 6 comments

Comments

@12089897411
Copy link

https://github.com/whisperfish/rust-phonenumber/blob/210d8df2116d34a224d97edcf3687c08e03437c8/src/national_number.rs#L28C6-L28C55

assert! cannot be used, please consider the following situation: Phone::validate_str("0xd2dc00a30027c9d5"), this process will cause the program to crash.

@rubdos
Copy link
Member

rubdos commented Apr 16, 2024

We do not have a struct Phone nor a function validate_str that accepts random hexadecimal numbers. We should probably, however, document the panicking behaviour, and clarify that national numbers larger than 56 bits do not exist to our knowledge.

@12089897411
Copy link
Author

Normally we deal with external untrusted characters. But the validate_str procedure can return Result<T,E> instead of crashing the program.

@rubdos
Copy link
Member

rubdos commented Apr 18, 2024

NationalNumber::new is not supposed to take untrusted data, and it certainly does not accept characters. The PhoneNumber::from_str implementation might parse into a number of more than 56 bits and feed it to NationalNumber::new. It might be interesting to find an actual case for that.

@realtimetodie
Copy link

This code is horrible and unsafe. People should not use this library.

@rubdos
Copy link
Member

rubdos commented Jul 7, 2024

This issue needs more context or an example crash, or any other evidence to be actionable. I'm closing this for now.

@rubdos rubdos closed this as completed Jul 7, 2024
rubdos added a commit that referenced this issue Jul 9, 2024
@rubdos
Copy link
Member

rubdos commented Jul 9, 2024

@12089897411 I started playing with proptests soon after I closed this issue, and the panic actually showed up in a code path. I've patched this as per your suggestion, bubbling up the error. Thanks for reporting.

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

No branches or pull requests

3 participants