Skip to content

Commit

Permalink
fix: relax Ident and ProcedureName validation, use checked
Browse files Browse the repository at this point in the history
constructors in deserialization.
  • Loading branch information
greenhat committed Jan 22, 2025
1 parent 8b60a31 commit 37fe048
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 45 deletions.
5 changes: 4 additions & 1 deletion assembly/src/ast/ident.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,10 @@ impl Ident {
if !source.starts_with(|c: char| c.is_ascii_alphabetic()) {
return Err(IdentError::InvalidStart);
}
if !source.chars().all(|c| c.is_ascii_alphabetic() || matches!(c, '_' | '0'..='9')) {
if !source
.chars()
.all(|c| c.is_ascii_alphanumeric() || matches!(c, '_' | '-' | ':' | '/' | '@' | '.'))
{
return Err(IdentError::InvalidChars { ident: source.into() });
}
Ok(())
Expand Down
25 changes: 11 additions & 14 deletions assembly/src/ast/procedure/name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,12 @@ impl FromStr for ProcedureName {
break Err(IdentError::InvalidChars { ident: s.into() });
}
},
Some((_, c)) if c.is_ascii_lowercase() || c == '_' || c == '$' => {
Some((_, c))
if c.is_ascii_lowercase() || c == '_' || c == '-' || c == '$' || c == '.' =>
{
if chars.as_str().contains(|c| match c {
c if c.is_ascii_alphanumeric() => false,
'_' | '$' => false,
'_' | '-' | '$' | '.' => false,
_ => true,
}) {
Err(IdentError::InvalidChars { ident: s.into() })
Expand All @@ -360,8 +362,8 @@ impl Serializable for ProcedureName {
impl Deserializable for ProcedureName {
fn read_from<R: ByteReader>(source: &mut R) -> Result<Self, DeserializationError> {
let str: String = source.read()?;
let proc_name =
ProcedureName::new_unchecked(Ident::new_unchecked(Span::unknown(Arc::from(str))));
let proc_name = ProcedureName::new(str)
.map_err(|e| DeserializationError::InvalidValue(e.to_string()))?;
Ok(proc_name)
}
}
Expand All @@ -377,16 +379,11 @@ impl proptest::prelude::Arbitrary for ProcedureName {
use proptest::prelude::*;
// see https://doc.rust-lang.org/rustc/symbol-mangling/v0.html#symbol-grammar-summary
let all_possible_chars_in_mangled_name =
"$abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.";
let mangled_rustc_name = ProcedureName::new_unchecked(Ident::new_unchecked(Span::new(
SourceSpan::UNKNOWN,
all_possible_chars_in_mangled_name.into(),
)));
let plain = ProcedureName::new_unchecked(Ident::new_unchecked(Span::new(
SourceSpan::UNKNOWN,
"userfunc".into(),
)));
prop_oneof![Just(mangled_rustc_name), Just(plain)].boxed()
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.$";
let mangled_rustc_name = ProcedureName::new(all_possible_chars_in_mangled_name).unwrap();
let plain = ProcedureName::new("user_func").unwrap();
let wasm_cm_style = ProcedureName::new("kebab-case-func").unwrap();
prop_oneof![Just(mangled_rustc_name), Just(plain), Just(wasm_cm_style)].boxed()
}

type Strategy = proptest::prelude::BoxedStrategy<Self>;
Expand Down
45 changes: 15 additions & 30 deletions assembly/src/library/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use core::{
};

use smallvec::smallvec;
use vm_core::debuginfo::SourceSpan;

use crate::{
ast::{Ident, IdentError},
Expand Down Expand Up @@ -506,26 +505,23 @@ impl Deserializable for LibraryPath {
let path = source.read_slice(len)?;
let path =
str::from_utf8(path).map_err(|e| DeserializationError::InvalidValue(e.to_string()))?;
let path = LibraryPath::new(path).unwrap_or_else(|_| {
LibraryPath::new(path).or_else(|_| {
// Try to parse at least the namespace
match LibraryNamespace::strip_path_prefix(path) {
Ok((ns, rest)) => {
let module_id = Ident::new_unchecked(Span::new(
SourceSpan::default(),
Arc::from(rest.to_string()),
));
LibraryPath::new_from_components(ns, [module_id])
let module_id = Ident::new(rest).map_err(|e| {
DeserializationError::InvalidValue(format!("Invalid module id: {}", e))
})?;
Ok(LibraryPath::new_from_components(ns, [module_id]))
},
Err(_) => {
let module_id = Ident::new_unchecked(Span::new(
SourceSpan::default(),
Arc::from(path.to_string()),
));
LibraryPath::new_from_components(LibraryNamespace::Anon, [module_id])
let module_id = Ident::new(path).map_err(|e| {
DeserializationError::InvalidValue(format!("Invalid module id: {}", e))
})?;
Ok(LibraryPath::new_from_components(LibraryNamespace::Anon, [module_id]))
},
}
});
Ok(path)
})
}
}

Expand Down Expand Up @@ -560,23 +556,18 @@ impl proptest::prelude::Arbitrary for LibraryPath {

fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
use proptest::prelude::*;

let wasm_cm_style = LibraryPath::new_from_components(
LibraryNamespace::Anon,
[Ident::new_unchecked(Span::new(
SourceSpan::UNKNOWN,
"namespace:package/[email protected]".into(),
))],
[Ident::new("namespace-kebab:package-kebab/[email protected]").unwrap()],
);
let path_len_2 = LibraryPath::new_from_components(
LibraryNamespace::User("userns".into()),
[Ident::new_unchecked(Span::new(SourceSpan::UNKNOWN, "usermodule".into()))],
LibraryNamespace::User("user_ns".into()),
[Ident::new("user_module").unwrap()],
);
let path_len_3 = LibraryPath::new_from_components(
LibraryNamespace::User("userns".into()),
[
Ident::new_unchecked(Span::new(SourceSpan::UNKNOWN, "userns2".into())),
Ident::new_unchecked(Span::new(SourceSpan::UNKNOWN, "usermodule".into())),
],
[Ident::new("user_path1").unwrap(), Ident::new("user_module").unwrap()],
);
prop_oneof![Just(wasm_cm_style), Just(path_len_2), Just(path_len_3)].boxed()
}
Expand Down Expand Up @@ -634,12 +625,6 @@ mod tests {
let path = LibraryPath::new("foo::1bar");
assert_matches!(path, Err(PathError::InvalidComponent(IdentError::InvalidStart)));

let path = LibraryPath::new("foo::b@r");
assert_matches!(
path,
Err(PathError::InvalidComponent(IdentError::InvalidChars { ident: _ }))
);

let path = LibraryPath::new("#foo::bar");
assert_matches!(
path,
Expand Down

0 comments on commit 37fe048

Please sign in to comment.