Merge rust-bitcoin/rust-bitcoin#6819: Remove panics from `LegacyPublicKey::from_str`
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6819: Remove panics from `LegacyPublicKey::from_str`
e9eebc4a49e32d157f1372c8f895243fba7cda4d Remove panics from `LegacyPublicKey::from_str` (Martin Habovstiak)
Pull request description:
While the implementation was correct the lack of panics was not obvious from the code. This rewrites it to be completely panic-free.
ACKs for top commit:
apoelstra:
ACK e9eebc4a49e32d157f1372c8f895243fba7cda4d; successfully ran local tests
Tree-SHA512: 353cf2043d059f2e120bd49eb88c83d0a3cde451a805f9f34bafd0bdc1641ffc17b4e000e1be9ff817a740fe48f3456598ec85e3941481a81ca0c6b6e66bfe5c
What changed, and why it matters
This change refactors how a Bitcoin public key is parsed from a text string so that the code no longer relies on 'this should never happen' assumptions that could trigger a program crash. The old code only accepted two exact string lengths and used an internal 'unreachable' panic for any other length error. The new code tries the two valid lengths directly and returns a proper error instead of panicking. It is a defensive hardening fix rather than a confirmed exploitable vulnerability.
Treat as a low-risk hardening improvement. Review downstream callers to ensure they handle ParsePublicKeyError rather than relying on panics, and consider auditing other FromStr implementations for similar unreachable! assumptions.
Security signals we found
Removal of unreachable! panic branches in parsing code
Defensive refactor to make panic-freedom locally obvious
Improved error handling for malformed input lengths
No change to accepted valid inputs or public API semantics
Evidence from the diff
LegacyPublicKey::from_str previously matched s.len() against 66 or 130, then called hex::decode_to_array with a fixed-size buffer and mapped DecodeFixedLengthBytesError::InvalidLength to unreachable! (panic). The patch replaces that with a generic try_decode::
Changed components
crypto/src/key.rsLegacyPublicKey::from_strInspect captured patch +10 / −19
### crypto/src/key.rs
@@ -829,27 +829,18 @@ impl FromStr for LegacyPublicKey {
type Err = ParsePublicKeyError;
#[inline]
fn from_str(s: &str) -> Result<Self, ParsePublicKeyError> {
- match s.len() {
- 66 => {
- let bytes = hex::decode_to_array::<33>(s).map_err(|e| match e {
- DecodeFixedLengthBytesError::InvalidChar(e) =>
- ParsePublicKeyError::InvalidChar(e),
- DecodeFixedLengthBytesError::InvalidLength(_) =>
- unreachable!("length checked already"),
- })?;
- Self::from_slice(&bytes).map_err(ParsePublicKeyError::Encoding)
+ fn try_decode<const N: usize>(s: &str) -> Result<LegacyPublicKey, ParsePublicKeyError> {
+ match hex::decode_to_array::<N>(s) {
+ Ok(bytes) => LegacyPublicKey::from_slice(&bytes).map_err(ParsePublicKeyError::Encoding),
+ Err(DecodeFixedLengthBytesError::InvalidChar(e)) => Err(ParsePublicKeyError::InvalidChar(e)),
+ Err(DecodeFixedLengthBytesError::InvalidLength(_)) => Err(ParsePublicKeyError::InvalidHexLength(s.len())),
}
- 130 => {
- let bytes = hex::decode_to_array::<65>(s).map_err(|e| match e {
- DecodeFixedLengthBytesError::InvalidChar(e) =>
- ParsePublicKeyError::InvalidChar(e),
- DecodeFixedLengthBytesError::InvalidLength(_) =>
- unreachable!("length checked already"),
- })?;
- Self::from_slice(&bytes).map_err(ParsePublicKeyError::Encoding)
- }
- len => Err(ParsePublicKeyError::InvalidHexLength(len)),
}
+ try_decode::<33>(s).or_else(|e| match e {
+ ParsePublicKeyError::InvalidChar(e) => Err(ParsePublicKeyError::InvalidChar(e)),
+ ParsePublicKeyError::Encoding(e) => Err(ParsePublicKeyError::Encoding(e)),
+ ParsePublicKeyError::InvalidHexLength(_) => try_decode::<65>(s),
+ })
}
}
Why this scored 27/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.