From 1698d0b63cc7291e32936569b378128eed87b629 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Mon, 1 Apr 2024 17:26:30 -0700 Subject: [PATCH 1/3] humility-core: allow enums with signed variants Fixes #468. --- cmd/rpc/src/lib.rs | 2 +- cmd/validate/src/lib.rs | 6 +- humility-core/src/hubris.rs | 193 +++++++++++++++++++++++++++++++++--- humility-hiffy/src/lib.rs | 10 +- humility-idol/src/lib.rs | 2 +- 5 files changed, 190 insertions(+), 23 deletions(-) diff --git a/cmd/rpc/src/lib.rs b/cmd/rpc/src/lib.rs index d5e56166e..35af60fa1 100644 --- a/cmd/rpc/src/lib.rs +++ b/cmd/rpc/src/lib.rs @@ -415,7 +415,7 @@ impl<'a> RpcClient<'a> { let buf = &self.buf[..n]; if buf[0] != 0 { - match self.rpc_reply_type.lookup_variant_by_tag(buf[0] as u64) { + match self.rpc_reply_type.lookup_variant_by_tag(Tag::from(buf[0])) { Some(e) => { let msg = format!("Got error from `udprpc`: {}", e.name); if e.name == "BadImageId" { diff --git a/cmd/validate/src/lib.rs b/cmd/validate/src/lib.rs index 81427b1b6..2432508e3 100644 --- a/cmd/validate/src/lib.rs +++ b/cmd/validate/src/lib.rs @@ -205,7 +205,9 @@ fn validate(context: &mut ExecutionContext) -> Result<()> { for (rndx, (ndx, device)) in devices.iter().enumerate() { let result = match &results[rndx] { Ok(val) => { - if let Some(variant) = ok.lookup_variant_by_tag(val[0].into()) { + if let Some(variant) = + ok.lookup_variant_by_tag(Tag::from(val[0])) + { Ok(match variant.name.as_str() { "Present" => "present".yellow(), "Validated" => "validated".green(), @@ -217,7 +219,7 @@ fn validate(context: &mut ExecutionContext) -> Result<()> { } Err(e) => { if let idol::IdolError::CLike(err) = op.error { - Ok(match err.lookup_variant_by_tag(*e as u64) { + Ok(match err.lookup_variant_by_tag(Tag::from(*e)) { Some(variant) => match variant.name.as_str() { "NotPresent" => { if device.removable { diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index f7fc4ed93..41029845f 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -15,6 +15,7 @@ use std::fmt::{self, Write}; use std::fs::{self, OpenOptions}; use std::io::Cursor; use std::mem::size_of; +use std::num::TryFromIntError; use std::path::Path; use std::str::{self, FromStr}; use std::time::Instant; @@ -4756,7 +4757,11 @@ impl HubrisObjectLoader { while let Some(attr) = attrs.next()? { if attr.name() == gimli::constants::DW_AT_discr_value { - value = attr.value().udata_value(); + value = attr.value().udata_value().map(Tag::Unsigned); + + if value.is_none() { + value = attr.value().sdata_value().map(Tag::Signed); + } if value.is_none() { bail!("bad discriminant on union {}", parent); @@ -4844,10 +4849,19 @@ impl HubrisObjectLoader { } gimli::constants::DW_AT_const_value => { - value = attr.value().udata_value(); + value = attr.value().udata_value().map(Tag::Unsigned); if value.is_none() { - bail!("bad discriminant on const enum {}", parent); + value = attr.value().sdata_value().map(Tag::Signed); + } + + if value.is_none() { + bail!( + "bad discriminant on const enum {}: {:?} ({:?})", + parent, + attr.value(), + name + ); } } @@ -5837,7 +5851,136 @@ pub struct HubrisEnumVariant { pub name: String, pub offset: usize, pub goff: Option, - pub tag: Option, + pub tag: Option, +} + +/// Type representing an enum variant tag. +#[derive(Copy, Clone, Debug, Eq)] +pub enum Tag { + Unsigned(u64), + Signed(i64), +} + +/// Equality for tags is a little weird. Most of Humility assumes enum +/// discriminants are unsigned. So, we want to treat an unsigned representation +/// of a signed number as equivalent. +impl PartialEq for Tag { + fn eq(&self, other: &Self) -> bool { + match (*self, *other) { + // These two lines are basically what derive(PartialEq would do. + (Self::Unsigned(a), Self::Unsigned(b)) => a == b, + (Self::Signed(a), Self::Signed(b)) => a == b, + + // This ensures that a signed discriminant, reinterpreted by + // Humility as an unsigned integer, is treated as equivalent. + (Self::Unsigned(u), Self::Signed(s)) + | (Self::Signed(s), Self::Unsigned(u)) => u == s as u64, + } + } +} + +/// All u64s can become Tags. +impl From for Tag { + fn from(u: u64) -> Self { + Tag::Unsigned(u) + } +} + +/// All u32s can become Tags. +impl From for Tag { + fn from(u: u32) -> Self { + Tag::Unsigned(u64::from(u)) + } +} + +/// All u16s can become Tags. +impl From for Tag { + fn from(u: u16) -> Self { + Tag::Unsigned(u64::from(u)) + } +} + +/// All u8s can become Tags. +impl From for Tag { + fn from(u: u8) -> Self { + Tag::Unsigned(u64::from(u)) + } +} + +/// All i64s can become Tags. +impl From for Tag { + fn from(i: i64) -> Self { + Tag::Signed(i) + } +} + +/// All i32s can become Tags. +impl From for Tag { + fn from(i: i32) -> Self { + Tag::Signed(i64::from(i)) + } +} + +/// All i16s can become Tags. +impl From for Tag { + fn from(i: i16) -> Self { + Tag::Signed(i64::from(i)) + } +} + +/// All i8s can become Tags. +impl From for Tag { + fn from(i: i8) -> Self { + Tag::Signed(i64::from(i)) + } +} + +/// Some Tags can become u8s. +impl TryFrom for u8 { + type Error = TryFromIntError; + + fn try_from(t: Tag) -> Result { + match t { + Tag::Unsigned(u) => u.try_into(), + Tag::Signed(i) => i.try_into(), + } + } +} + +/// Some Tags can become u16s. +impl TryFrom for u16 { + type Error = TryFromIntError; + + fn try_from(t: Tag) -> Result { + match t { + Tag::Unsigned(u) => u.try_into(), + Tag::Signed(i) => i.try_into(), + } + } +} + +/// Some Tags can become u32s. +impl TryFrom for u32 { + type Error = TryFromIntError; + + fn try_from(t: Tag) -> Result { + match t { + Tag::Unsigned(u) => u.try_into(), + Tag::Signed(i) => i.try_into(), + } + } +} + +/// Some Tags (and all unsigned Tags) can become u64s. +impl TryFrom for u64 { + type Error = TryFromIntError; + + fn try_from(t: Tag) -> Result { + match t { + Tag::Unsigned(u) => Ok(u), + Tag::Signed(i) => i.try_into(), + } + } } #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -5855,7 +5998,7 @@ pub struct HubrisEnum { /// only one variant. pub discriminant: Option, /// temporary to hold tag of next variant - pub tag: Option, + pub tag: Option, pub variants: Vec, namespace: Option, } @@ -5863,7 +6006,7 @@ pub struct HubrisEnum { impl HubrisEnum { pub fn lookup_variant_by_tag( &self, - tag: u64, + tag: Tag, ) -> Option<&HubrisEnumVariant> { // We prioritize picking a variant with the matching tag if let Some(t) = self.variants.iter().find(|v| v.tag == Some(tag)) { @@ -5905,12 +6048,30 @@ impl HubrisEnum { hubris: &HubrisArchive, buf: &[u8], ) -> Result<&HubrisEnumVariant> { - let readval = |b: &[u8], o, sz| -> Result { - Ok(match sz { - 1 => u64::from(b[o]), - 2 => u64::from(u16::from_le_bytes(b[o..o + 2].try_into()?)), - 4 => u64::from(u32::from_le_bytes(b[o..o + 4].try_into()?)), - 8 => u64::from_le_bytes(b[o..o + 8].try_into()?), + let readtag = |b: &[u8], o, sz, enc| -> Result { + Ok(match (sz, enc) { + (1, HubrisEncoding::Unsigned) => Tag::from(b[o]), + (2, HubrisEncoding::Unsigned) => { + Tag::from(u16::from_le_bytes(b[o..o + 2].try_into()?)) + } + (4, HubrisEncoding::Unsigned) => { + Tag::from(u32::from_le_bytes(b[o..o + 4].try_into()?)) + } + (8, HubrisEncoding::Unsigned) => { + Tag::from(u64::from_le_bytes(b[o..o + 8].try_into()?)) + } + + (1, HubrisEncoding::Signed) => Tag::from(b[o] as i8), + (2, HubrisEncoding::Signed) => { + Tag::from(i16::from_le_bytes(b[o..o + 2].try_into()?)) + } + (4, HubrisEncoding::Signed) => { + Tag::from(i32::from_le_bytes(b[o..o + 4].try_into()?)) + } + (8, HubrisEncoding::Signed) => { + Tag::from(i64::from_le_bytes(b[o..o + 8].try_into()?)) + } + _ => { bail!("bad variant size!"); } @@ -5918,18 +6079,18 @@ impl HubrisEnum { }; if let Some(HubrisDiscriminant::Value(goff, offs)) = self.discriminant { - let size = match hubris.basetypes.get(&goff) { - Some(v) => v.size, + let (encoding, size) = match hubris.basetypes.get(&goff) { + Some(v) => (v.encoding, v.size), None => { bail!("enum has discriminant of unknown type: {}", goff); } }; - let val = readval(buf, offs, size)?; + let val = readtag(buf, offs, size, encoding)?; match self.lookup_variant_by_tag(val) { None => { - bail!("unknown variant: 0x{:x}", val); + bail!("unknown variant: {:#x?}", val); } Some(variant) => Ok(variant), diff --git a/humility-hiffy/src/lib.rs b/humility-hiffy/src/lib.rs index 86864b354..c563005cb 100644 --- a/humility-hiffy/src/lib.rs +++ b/humility-hiffy/src/lib.rs @@ -692,7 +692,9 @@ impl<'a> HiffyContext<'a> { // if buf[0] != 0 { let rpc_reply_type = self.rpc_reply_type.unwrap(); - match rpc_reply_type.lookup_variant_by_tag(buf[0] as u64) { + match rpc_reply_type + .lookup_variant_by_tag(Tag::from(buf[0])) + { Some(e) => { let image_id = self.hubris.image_id().unwrap(); let msg = format!("RPC error: {}", e.name); @@ -881,7 +883,7 @@ impl<'a> HiffyContext<'a> { } Err(e) => { let variant = if let idol::IdolError::CLike(error) = op.error { - error.lookup_variant_by_tag(*e as u64) + error.lookup_variant_by_tag(Tag::from(*e as u64)) } else { None }; @@ -1296,7 +1298,9 @@ pub fn hiffy_decode( } Err(e) => match op.error { idol::IdolError::CLike(error) => { - if let Some(v) = error.lookup_variant_by_tag(e as u64) { + if let Some(v) = + error.lookup_variant_by_tag(Tag::from(e as u64)) + { Err(v.name.to_string()) } else { Err(format!("")) diff --git a/humility-idol/src/lib.rs b/humility-idol/src/lib.rs index 3b371a253..4122dcd2a 100644 --- a/humility-idol/src/lib.rs +++ b/humility-idol/src/lib.rs @@ -195,7 +195,7 @@ impl<'a> IdolOperation<'a> { pub fn strerror(&self, code: u32) -> String { let variant = if let IdolError::CLike(error) = self.error { - error.lookup_variant_by_tag(code as u64) + error.lookup_variant_by_tag(Tag::from(code as u64)) } else { None }; From 9efbc8a878718baa596c2d0d96bc07e1755af711 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Tue, 2 Apr 2024 11:10:50 -0700 Subject: [PATCH 2/3] Note sites where fix could not be applied, add doc comments --- cmd/rpc/src/lib.rs | 3 +++ cmd/validate/src/lib.rs | 8 +++++++ humility-core/src/hubris.rs | 42 ++++++++++++++++++++----------------- humility-hiffy/src/lib.rs | 12 +++++++++++ humility-idol/src/lib.rs | 4 ++++ 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/cmd/rpc/src/lib.rs b/cmd/rpc/src/lib.rs index 35af60fa1..651837789 100644 --- a/cmd/rpc/src/lib.rs +++ b/cmd/rpc/src/lib.rs @@ -415,6 +415,9 @@ impl<'a> RpcClient<'a> { let buf = &self.buf[..n]; if buf[0] != 0 { + // TODO: assumes the discriminator is a u8. It's not clear from + // context whether this assumption carries through into the udprpc + // task. match self.rpc_reply_type.lookup_variant_by_tag(Tag::from(buf[0])) { Some(e) => { let msg = format!("Got error from `udprpc`: {}", e.name); diff --git a/cmd/validate/src/lib.rs b/cmd/validate/src/lib.rs index 2432508e3..5801b952a 100644 --- a/cmd/validate/src/lib.rs +++ b/cmd/validate/src/lib.rs @@ -205,6 +205,10 @@ fn validate(context: &mut ExecutionContext) -> Result<()> { for (rndx, (ndx, device)) in devices.iter().enumerate() { let result = match &results[rndx] { Ok(val) => { + // TODO: assumes discriminant is a u8. Since this is using Hiffy + // call results instead of looking at a Rust value in memory, + // it's not clear from context what changes would be required to + // fix this. if let Some(variant) = ok.lookup_variant_by_tag(Tag::from(val[0])) { @@ -219,6 +223,10 @@ fn validate(context: &mut ExecutionContext) -> Result<()> { } Err(e) => { if let idol::IdolError::CLike(err) = op.error { + // TODO: assumes discriminant is a u8. Since this is using + // Hiffy call results instead of looking at a Rust value in + // memory, it's not clear from context what changes would be + // required to fix this. Ok(match err.lookup_variant_by_tag(Tag::from(*e)) { Some(variant) => match variant.name.as_str() { "NotPresent" => { diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index 41029845f..953a97136 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -5855,30 +5855,18 @@ pub struct HubrisEnumVariant { } /// Type representing an enum variant tag. -#[derive(Copy, Clone, Debug, Eq)] +/// +/// Tags may be signed or unsigned. Every variant of a given enum uses the same +/// signedness. (This should likely be available on the HubrisEnum type but is +/// not.) As a result, signed and unsigned `Tag`s never compare as equal. This +/// can pose a problem for manually rolling enum reconstruction code using +/// `lookup_variant_by_tag`, but `determine_variant` will do the right thing. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum Tag { Unsigned(u64), Signed(i64), } -/// Equality for tags is a little weird. Most of Humility assumes enum -/// discriminants are unsigned. So, we want to treat an unsigned representation -/// of a signed number as equivalent. -impl PartialEq for Tag { - fn eq(&self, other: &Self) -> bool { - match (*self, *other) { - // These two lines are basically what derive(PartialEq would do. - (Self::Unsigned(a), Self::Unsigned(b)) => a == b, - (Self::Signed(a), Self::Signed(b)) => a == b, - - // This ensures that a signed discriminant, reinterpreted by - // Humility as an unsigned integer, is treated as equivalent. - (Self::Unsigned(u), Self::Signed(s)) - | (Self::Signed(s), Self::Unsigned(u)) => u == s as u64, - } - } -} - /// All u64s can become Tags. impl From for Tag { fn from(u: u64) -> Self { @@ -6004,6 +5992,16 @@ pub struct HubrisEnum { } impl HubrisEnum { + /// Finds a variant of this enum type that has the discriminant `tag`. + /// + /// `tag` must have the proper signedness for this enum. Historically a lot + /// of code assumed that discriminants were always unsigned, and there are + /// still routines around that make this assumption; they will simply fail + /// to find variants for enums using signed discriminants. + /// + /// If you would like to avoid this pitfall, the `determine_variant` + /// routine will do the type lookup for you, and should almost always be + /// preferred where applicable! pub fn lookup_variant_by_tag( &self, tag: Tag, @@ -6043,6 +6041,12 @@ impl HubrisEnum { bail!("missing variant: {}.{}", self.name, name) } + /// Finds a variant of this enum given a blob of data representing one of + /// its variants. This handles looking up the offset of the tag in memory, + /// and handling signed/unsigned tags. + /// + /// It's almost always better to use this than to attempt to roll your own + /// using `lookup_variant_by_tag`. pub fn determine_variant( &self, hubris: &HubrisArchive, diff --git a/humility-hiffy/src/lib.rs b/humility-hiffy/src/lib.rs index c563005cb..46a61f6a8 100644 --- a/humility-hiffy/src/lib.rs +++ b/humility-hiffy/src/lib.rs @@ -692,6 +692,9 @@ impl<'a> HiffyContext<'a> { // if buf[0] != 0 { let rpc_reply_type = self.rpc_reply_type.unwrap(); + // TODO: this assumes that the reply enum can be represented + // by a u8 (buf[0] is a u8) and will not work with larger + // discriminants, or signed discriminants. match rpc_reply_type .lookup_variant_by_tag(Tag::from(buf[0])) { @@ -883,6 +886,11 @@ impl<'a> HiffyContext<'a> { } Err(e) => { let variant = if let idol::IdolError::CLike(error) = op.error { + // TODO potentially sign-extended discriminator represented + // as u32 and then zero-extended to u64; won't work for + // signed values. Can't use determine_variant here because + // it's not laid out in memory, it's been unfolded onto the + // return stack. error.lookup_variant_by_tag(Tag::from(*e as u64)) } else { None @@ -1298,6 +1306,10 @@ pub fn hiffy_decode( } Err(e) => match op.error { idol::IdolError::CLike(error) => { + // TODO potentially sign-extended discriminator represented as + // u32 and then zero-extended to u64; won't work for signed + // values. Can't use determine_variant here because it's not + // laid out in memory, it's been unfolded onto the return stack. if let Some(v) = error.lookup_variant_by_tag(Tag::from(e as u64)) { diff --git a/humility-idol/src/lib.rs b/humility-idol/src/lib.rs index 4122dcd2a..7ac52056a 100644 --- a/humility-idol/src/lib.rs +++ b/humility-idol/src/lib.rs @@ -195,6 +195,10 @@ impl<'a> IdolOperation<'a> { pub fn strerror(&self, code: u32) -> String { let variant = if let IdolError::CLike(error) = self.error { + // TODO: assumes discriminant is a u8. Since this is using Hiffy + // call results instead of looking at a Rust value in memory, it's + // not clear from context what changes would be required to fix + // this. error.lookup_variant_by_tag(Tag::from(code as u64)) } else { None From 22a4b6f733086bb82553ca34daec4996b4bbaa44 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Wed, 3 Apr 2024 14:10:35 -0700 Subject: [PATCH 3/3] Bump minor version to reflect enum support. --- Cargo.lock | 2 +- Cargo.toml | 2 +- tests/cmd/chip.trycmd | 4 ++-- tests/cmd/version.trycmd | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 69475979f..e716258e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1195,7 +1195,7 @@ dependencies = [ [[package]] name = "humility" -version = "0.11.1" +version = "0.11.2" dependencies = [ "anyhow", "bitfield", diff --git a/Cargo.toml b/Cargo.toml index e1a67b137..e09f2a757 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,7 @@ name = "humility" # # Be sure to check in and push all of the files that change. Happy versioning! # -version = "0.11.1" +version = "0.11.2" authors = ["Bryan Cantrill "] edition = "2018" license = "MPL-2.0" diff --git a/tests/cmd/chip.trycmd b/tests/cmd/chip.trycmd index 5bd3b8049..7f64c1a57 100644 --- a/tests/cmd/chip.trycmd +++ b/tests/cmd/chip.trycmd @@ -13,7 +13,7 @@ For more information try --help ``` $ humility --chip this-can-be-anything -V -humility 0.11.1 +humility 0.11.2 ``` @@ -28,7 +28,7 @@ For more information try --help ``` $ humility -c apx432 -V -humility 0.11.1 +humility 0.11.2 ``` diff --git a/tests/cmd/version.trycmd b/tests/cmd/version.trycmd index 52dd20bad..8c4697f6b 100644 --- a/tests/cmd/version.trycmd +++ b/tests/cmd/version.trycmd @@ -2,7 +2,7 @@ Long version flag: ``` $ humility --version -humility 0.11.1 +humility 0.11.2 ``` @@ -10,6 +10,6 @@ Short version flag: ``` $ humility -V -humility 0.11.1 +humility 0.11.2 ```