idl-spec: avoid panics on malformed defined type generics#4246
Conversation
|
Someone is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the IdlType::from_str parsing logic to prevent panics when processing malformed type strings with generics and arrays. Previously, malformed inputs like MyStruct<Pubkey (missing closing >) or [u8 32] (missing semicolon) would trigger panics due to unwrap() usage in the parsing code.
Changes:
- Replaced
unwrap()calls with structuredanyhow::Errorreturns in array and defined type generic parsing - Added comprehensive error messages for malformed type syntax
- Added test cases to verify malformed inputs return errors instead of panicking
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let name = s | ||
| .get(..i) | ||
| .ok_or_else(|| anyhow!("Invalid defined type '{s}': missing name"))? | ||
| .to_owned(); |
There was a problem hiding this comment.
The error check here is unreachable. Since i comes from s.find('<') on line 388, it's guaranteed to be a valid index into s. The call to s.get(..i) will always return Some, making the .ok_or_else() check unnecessary. Consider using direct slicing s[..i].to_owned() or removing the error check.
| let name = s | |
| .get(..i) | |
| .ok_or_else(|| anyhow!("Invalid defined type '{s}': missing name"))? | |
| .to_owned(); | |
| let name = s[..i].to_owned(); |
| let generics_str = s | ||
| .get(i + 1..) | ||
| .ok_or_else(|| anyhow!("Invalid defined type '{s}': missing generics"))? |
There was a problem hiding this comment.
The error check here is unreachable. Since i comes from s.find('<') on line 388, we know that i + 1 will be at most s.len() (if '<' is at the end). The call to s.get(i + 1..) will always return Some (possibly an empty string), making the .ok_or_else() check unnecessary. Consider using direct slicing s[i + 1..] or removing the error check.
| let generics_str = s | |
| .get(i + 1..) | |
| .ok_or_else(|| anyhow!("Invalid defined type '{s}': missing generics"))? | |
| let generics_str = s[i + 1..] |
| if g.is_empty() { | ||
| return Err(anyhow!( | ||
| "Invalid defined type '{s}': empty generic argument" | ||
| )); | ||
| } |
There was a problem hiding this comment.
The empty generic argument check on line 408 will catch trailing commas in generic lists (e.g., "MyStruct<Pubkey,>"), which is good. However, it would also catch multiple consecutive commas (e.g., "MyStruct<Pubkey,,u64>"). Consider adding a test case for trailing commas to document this behavior.
| .ok_or_else(|| anyhow!("Invalid defined type '{s}': missing name"))? | ||
| .to_owned(); | ||
|
|
||
| let generics_str = s | ||
| .get(i + 1..) | ||
| .ok_or_else(|| anyhow!("Invalid defined type '{s}': missing generics"))? | ||
| .strip_suffix('>') | ||
| .ok_or_else(|| anyhow!("Invalid defined type '{s}': missing closing '>'"))?; |
There was a problem hiding this comment.
The error messages for defined types are more detailed than the existing error messages for Option and Vec types. For consistency, consider either:
- Making the defined type errors simpler (like "Invalid defined type"), or
- Improving the Option/Vec error messages to be similarly detailed (like "Invalid Option type '{s}': missing closing '>'")
This would make error messages more consistent across the codebase.
| fn malformed_defined_empty_generics_returns_err() { | ||
| // Previously this would panic due to `.strip_suffix('>').unwrap()` (and/or parse weirdly). | ||
| assert!(IdlType::from_str("MyStruct<>").is_err()); | ||
| } |
There was a problem hiding this comment.
Consider adding a test case for nested generic types with malformed syntax, such as "MyStruct<Option" (missing two closing angle brackets). This would verify that the recursive parsing in generic arguments properly propagates errors.
| } | |
| } | |
| #[test] | |
| fn malformed_defined_nested_missing_closing_angles_returns_err() { | |
| // Ensure recursive parsing of generic arguments propagates syntax errors from nested generics. | |
| assert!(IdlType::from_str("MyStruct<Option<Pubkey>").is_err()); | |
| } |
| #[test] | ||
| fn malformed_array_missing_semicolon_returns_err() { | ||
| // Previously this would panic due to `.rsplit_once(';').unwrap()`. | ||
| assert!(IdlType::from_str("[u8 32]").is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn malformed_array_invalid_element_type_returns_err() { | ||
| // Previously this would panic due to `IdlType::from_str(raw_type).unwrap()`. | ||
| // Use a type that is *syntactically* invalid (missing '>') so `from_str` returns Err. | ||
| assert!(IdlType::from_str("[Option<Pubkey; 32]").is_err()); | ||
| } |
There was a problem hiding this comment.
Consider adding a test case for malformed multidimensional arrays, such as "[[u8; 16]; 32" (missing closing ']'). This would verify that errors are properly propagated through the recursive array_from_str calls.
|
This appears to conflict with #4247, with both rewriting |
This PR hardens
anchor-lang-idl-spec'sIdlType::from_strparsing for defined types with generics.Issue
Malformed type strings like
MyStruct<Pubkey(missing closing>) would trigger a panic due tounwrap()usage in the generic parsing branch.Fix
unwrap()calls with structuredanyhow::Errorreturns.Err(and do not panic).Verification
cargo test -p anchor-lang-idl-specWrite-up: https://rentry.co/anchor-idl-defined-generics-dos-2