From 4fd3447f95f7b2935717b10702e7b816a3edd4ea Mon Sep 17 00:00:00 2001 From: molpopgen Date: Thu, 24 Mar 2022 14:31:41 -0700 Subject: [PATCH 1/6] Handle cases of cast_sign_loss. --- src/lib.rs | 6 +++--- src/metadata.rs | 15 ++++++++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 01d2a0fc0..faddcf8a5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -290,9 +290,9 @@ impl TryFrom for SizeType { type Error = crate::TskitError; fn try_from(value: tsk_id_t) -> Result { - match value >= 0 { - true => Ok(Self(value as crate::bindings::tsk_size_t)), - false => Err(crate::TskitError::RangeError(stringify!(value.0))), + match tsk_size_t::try_from(value) { + Ok(v) => Ok(Self(v)), + Err(_) => Err(crate::TskitError::RangeError(stringify!(value.0))), } } } diff --git a/src/metadata.rs b/src/metadata.rs index 9d1f14dac..a1ecec1d0 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -257,15 +257,23 @@ pub(crate) fn char_column_to_vector( num_rows: tsk_size_t, column_length: tsk_size_t, ) -> Result>, crate::TskitError> { - if row < 0 || (row as tsk_size_t) >= num_rows { + let row = match tsk_size_t::try_from(row) { + Ok(r) => r, + Err(_) => return Err(TskitError::IndexError), + }; + if row >= num_rows { return Err(crate::TskitError::IndexError {}); } if column_length == 0 { return Ok(None); } - let start = unsafe { *column_offset.offset(row as isize) }; + let row_isize = match isize::try_from(row) { + Ok(r) => r, + Err(_) => return Err(TskitError::RangeError("could not convert u64 to isize")), + }; + let start = unsafe { *column_offset.offset(row_isize) }; let stop = if (row as tsk_size_t) < num_rows { - unsafe { *column_offset.offset((row + 1) as isize) } + unsafe { *column_offset.offset(row_isize + 1) } } else { column_length }; @@ -278,6 +286,7 @@ pub(crate) fn char_column_to_vector( let mut buffer = vec![]; for i in start..stop { match isize::try_from(i) { + // NOTE: cast_sign_loss pedantic lint is a false +ve here. Ok(o) => buffer.push(unsafe { *column.offset(o) } as u8), Err(_) => return Err(TskitError::RangeError("could not convert value to isize")), }; From df4c2d8943d7ee8f7b0d55a656ff866afc3ba245 Mon Sep 17 00:00:00 2001 From: molpopgen Date: Fri, 25 Mar 2022 09:40:03 -0700 Subject: [PATCH 2/6] RangeError now holds String rather that &'static str. (This change allows nicer error messages.) --- src/error.rs | 2 +- src/lib.rs | 6 ++++-- src/metadata.rs | 14 ++++++++++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/error.rs b/src/error.rs index 3e57ae04f..e55378d97 100644 --- a/src/error.rs +++ b/src/error.rs @@ -7,7 +7,7 @@ use thiserror::Error; pub enum TskitError { /// Returned when conversion attempts fail #[error("range error: {}", *.0)] - RangeError(&'static str), + RangeError(String), /// Used when bad input is encountered. #[error("we received {} but expected {}",*got, *expected)] ValueError { got: String, expected: String }, diff --git a/src/lib.rs b/src/lib.rs index faddcf8a5..f24e9bc14 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -292,7 +292,9 @@ impl TryFrom for SizeType { fn try_from(value: tsk_id_t) -> Result { match tsk_size_t::try_from(value) { Ok(v) => Ok(Self(v)), - Err(_) => Err(crate::TskitError::RangeError(stringify!(value.0))), + Err(_) => Err(crate::TskitError::RangeError( + stringify!(value.0).to_string(), + )), } } } @@ -302,7 +304,7 @@ impl TryFrom for tsk_id_t { fn try_from(value: SizeType) -> Result { if value.0 > tsk_id_t::MAX as tsk_size_t { - Err(TskitError::RangeError(stringify!(value.0))) + Err(TskitError::RangeError(stringify!(value.0).to_string())) } else { Ok(value.0 as tsk_id_t) } diff --git a/src/metadata.rs b/src/metadata.rs index a1ecec1d0..e42a85658 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -269,7 +269,12 @@ pub(crate) fn char_column_to_vector( } let row_isize = match isize::try_from(row) { Ok(r) => r, - Err(_) => return Err(TskitError::RangeError("could not convert u64 to isize")), + Err(_) => { + return Err(TskitError::RangeError(format!( + "could not convert u64 value {} to isize", + stringify!(row) + ))) + } }; let start = unsafe { *column_offset.offset(row_isize) }; let stop = if (row as tsk_size_t) < num_rows { @@ -288,7 +293,12 @@ pub(crate) fn char_column_to_vector( match isize::try_from(i) { // NOTE: cast_sign_loss pedantic lint is a false +ve here. Ok(o) => buffer.push(unsafe { *column.offset(o) } as u8), - Err(_) => return Err(TskitError::RangeError("could not convert value to isize")), + Err(_) => { + return Err(TskitError::RangeError(format!( + "cauld not convert value {} to isize", + stringify!(i) + ))) + } }; } Ok(Some(buffer)) From 59f81f7d382eb2767d59e22e44c18f6b981981b2 Mon Sep 17 00:00:00 2001 From: molpopgen Date: Fri, 25 Mar 2022 09:49:45 -0700 Subject: [PATCH 3/6] Suppress cast_sign_loss pedantic lint when restoring metadata from the C side. --- src/metadata.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/metadata.rs b/src/metadata.rs index e42a85658..81b100cb1 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -292,6 +292,9 @@ pub(crate) fn char_column_to_vector( for i in start..stop { match isize::try_from(i) { // NOTE: cast_sign_loss pedantic lint is a false +ve here. + // The metadata live as C strings on the tskit-C side, so + // the integer cast exists as part of the round trip. + #[allow(clippy::cast_sign_loss)] Ok(o) => buffer.push(unsafe { *column.offset(o) } as u8), Err(_) => { return Err(TskitError::RangeError(format!( From 5c2611bf237fad8af20883bdac13c3423bf9caba Mon Sep 17 00:00:00 2001 From: molpopgen Date: Fri, 25 Mar 2022 10:26:11 -0700 Subject: [PATCH 4/6] Handle possibility that u64 cannot convert to usize on 32-bit systems. When this case arises, panic!. --- src/lib.rs | 2 +- src/node_table.rs | 9 +++++++-- src/table_collection.rs | 4 ++-- src/trees.rs | 2 +- src/util.rs | 30 ++++++++++++++++++++++++++++++ 5 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f24e9bc14..443c65a6b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -276,7 +276,7 @@ impl From for tsk_size_t { impl From for usize { fn from(value: SizeType) -> Self { - value.0 as usize + crate::util::handle_u64_to_usize(value.0) } } diff --git a/src/node_table.rs b/src/node_table.rs index 9a54aa730..787dcba09 100644 --- a/src/node_table.rs +++ b/src/node_table.rs @@ -112,7 +112,12 @@ impl<'a> NodeTable<'a> { /// Mutable access to node flags. pub fn flags_array_mut(&mut self) -> &mut [tsk_flags_t] { - unsafe { std::slice::from_raw_parts_mut(self.table_.flags, self.table_.num_rows as usize) } + unsafe { + std::slice::from_raw_parts_mut( + self.table_.flags, + crate::util::handle_u64_to_usize(self.table_.num_rows), + ) + } } /// Mutable access to node times. @@ -120,7 +125,7 @@ impl<'a> NodeTable<'a> { unsafe { std::slice::from_raw_parts_mut( self.table_.time.cast::