search

whisperfish / rust-phonenumber

Library for parsing, formatting and validating international phone numbers.
Apache License 2.0
162 stars 55 forks source link

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

Closed 12089897411 closed 3 months ago

12089897411 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

rubdos commented 6 months ago

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 commented 3 months ago

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

rubdos commented 3 months ago

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

rubdos commented 3 months ago

@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.