diff --git a/src/_macros.rs b/src/_macros.rs index 5ef3af793..64b9ad5de 100644 --- a/src/_macros.rs +++ b/src/_macros.rs @@ -28,73 +28,69 @@ macro_rules! panic_on_tskit_error { } macro_rules! unsafe_tsk_column_access { - ($i: expr, $lo: expr, $hi: expr, $array: expr) => {{ + ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident) => {{ if $i < $lo || ($i as $crate::tsk_size_t) >= $hi { None } else { - Some(unsafe { *$array.offset($i as isize) }) + debug_assert!(!($owner).$array.is_null()); + if !$owner.$array.is_null() { + // SAFETY: array is not null + // and we did our best effort + // on bounds checking + Some(unsafe { *$owner.$array.offset($i as isize) }) + } else { + None + } } }}; - ($i: expr, $lo: expr, $hi: expr, $array: expr, $output_id_type: expr) => {{ + ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident, $output_id_type: expr) => {{ if $i < $lo || ($i as $crate::tsk_size_t) >= $hi { None } else { - Some($output_id_type(unsafe { *$array.offset($i as isize) })) + debug_assert!(!($owner).$array.is_null()); + if !$owner.$array.is_null() { + // SAFETY: array is not null + // and we did our best effort + // on bounds checking + unsafe { Some($output_id_type(*($owner.$array.offset($i as isize)))) } + } else { + None + } } }}; } macro_rules! unsafe_tsk_column_access_and_map_into { - ($i: expr, $lo: expr, $hi: expr, $array: expr) => {{ - unsafe_tsk_column_access!($i, $lo, $hi, $array).map(|v| v.into()) + ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident) => {{ + unsafe_tsk_column_access!($i, $lo, $hi, $owner, $array).map(|v| v.into()) }}; } macro_rules! unsafe_tsk_ragged_column_access { - ($i: expr, $lo: expr, $hi: expr, $array: expr, $offset_array: expr, $offset_array_len: expr) => {{ + ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident, $offset_array: ident, $offset_array_len: ident, $output_id_type: ty) => {{ let i = $crate::SizeType::try_from($i).ok()?; if $i < $lo || i >= $hi { None - } else if $offset_array_len == 0 { + } else if $owner.$offset_array_len == 0 { None } else { - let start = unsafe { *$offset_array.offset($i as isize) }; - let stop = if i < $hi { - unsafe { *$offset_array.offset(($i + 1) as isize) } - } else { - $offset_array_len as tsk_size_t - }; - if start == stop { - None - } else { - let mut buffer = vec![]; - for i in start..stop { - buffer.push(unsafe { *$array.offset(i as isize) }); - } - Some(buffer) + debug_assert!(!$owner.$array.is_null()); + if $owner.$array.is_null() { + return None; } - } - }}; - - ($i: expr, $lo: expr, $hi: expr, $array: expr, $offset_array: expr, $offset_array_len: expr, $output_id_type: ty) => {{ - let i = $crate::SizeType::try_from($i).ok()?; - if $i < $lo || i >= $hi { - None - } else if $offset_array_len == 0 { - None - } else { - let start = unsafe { *$offset_array.offset($i as isize) }; + // SAFETY: we have checked bounds and ensured not null + let start = unsafe { *$owner.$offset_array.offset($i as isize) }; let stop = if i < $hi { - unsafe { *$offset_array.offset(($i + 1) as isize) } + unsafe { *$owner.$offset_array.offset(($i + 1) as isize) } } else { - $offset_array_len as tsk_size_t + $owner.$offset_array_len as tsk_size_t }; if start == stop { None } else { Some(unsafe { std::slice::from_raw_parts( - $array.offset(start as isize) as *const $output_id_type, + $owner.$array.offset(start as isize) as *const $output_id_type, stop as usize - start as usize, ) }) @@ -107,25 +103,27 @@ macro_rules! unsafe_tsk_ragged_column_access { // to pass clippy checks #[allow(unused_macros)] macro_rules! unsafe_tsk_ragged_char_column_access { - ($i: expr, $lo: expr, $hi: expr, $array: expr, $offset_array: expr, $offset_array_len: expr) => {{ + ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident, $offset_array: ident, $offset_array_len: ident) => {{ let i = $crate::SizeType::try_from($i)?; if $i < $lo || i >= $hi { Err(TskitError::IndexError {}) - } else if $offset_array_len == 0 { + } else if $owner.$offset_array_len == 0 { Ok(None) } else { - let start = unsafe { *$offset_array.offset($i as isize) }; + assert!(!$owner.$array.is_null()); + assert!(!$owner.$offset_array.is_null()); + let start = unsafe { *$owner.$offset_array.offset($i as isize) }; let stop = if i < $hi { - unsafe { *$offset_array.offset(($i + 1) as isize) } + unsafe { *$owner.$offset_array.offset(($i + 1) as isize) } } else { - $offset_array_len as tsk_size_t + $owner.$offset_array_len as tsk_size_t }; if start == stop { Ok(None) } else { let mut buffer = String::new(); for i in start..stop { - buffer.push(unsafe { *$array.offset(i as isize) as u8 as char }); + buffer.push(unsafe { *$owner.$array.offset(i as isize) as u8 as char }); } Ok(Some(buffer)) } diff --git a/src/edge_table.rs b/src/edge_table.rs index 21def03a8..7482581e6 100644 --- a/src/edge_table.rs +++ b/src/edge_table.rs @@ -89,7 +89,14 @@ impl<'a> EdgeTable<'a> { /// * `Some(parent)` if `u` is valid. /// * `None` otherwise. pub fn parent + Copy>(&'a self, row: E) -> Option { - unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_.parent, NodeId) + unsafe_tsk_column_access!( + row.into().0, + 0, + self.num_rows(), + self.table_, + parent, + NodeId + ) } /// Return the ``child`` value from row ``row`` of the table. @@ -99,7 +106,7 @@ impl<'a> EdgeTable<'a> { /// * `Some(child)` if `u` is valid. /// * `None` otherwise. pub fn child + Copy>(&'a self, row: E) -> Option { - unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_.child, NodeId) + unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_, child, NodeId) } /// Return the ``left`` value from row ``row`` of the table. @@ -109,7 +116,14 @@ impl<'a> EdgeTable<'a> { /// * `Some(position)` if `u` is valid. /// * `None` otherwise. pub fn left + Copy>(&'a self, row: E) -> Option { - unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_.left, Position) + unsafe_tsk_column_access!( + row.into().0, + 0, + self.num_rows(), + self.table_, + left, + Position + ) } /// Return the ``right`` value from row ``row`` of the table. @@ -119,7 +133,7 @@ impl<'a> EdgeTable<'a> { /// * `Some(position)` if `u` is valid. /// * `None` otherwise. pub fn right + Copy>(&'a self, row: E) -> Option { - unsafe_tsk_column_access_and_map_into!(row.into().0, 0, self.num_rows(), self.table_.right) + unsafe_tsk_column_access_and_map_into!(row.into().0, 0, self.num_rows(), self.table_, right) } pub fn metadata( diff --git a/src/individual_table.rs b/src/individual_table.rs index d8f1e5e5b..236a8dcf1 100644 --- a/src/individual_table.rs +++ b/src/individual_table.rs @@ -107,7 +107,7 @@ impl<'a> IndividualTable<'a> { /// * `Some(flags)` if `row` is valid. /// * `None` otherwise. pub fn flags + Copy>(&self, row: I) -> Option { - unsafe_tsk_column_access_and_map_into!(row.into().0, 0, self.num_rows(), self.table_.flags) + unsafe_tsk_column_access_and_map_into!(row.into().0, 0, self.num_rows(), self.table_, flags) } /// Return the locations for a given row. @@ -121,9 +121,10 @@ impl<'a> IndividualTable<'a> { row.into().0, 0, self.num_rows(), - self.table_.location, - self.table_.location_offset, - self.table_.location_length, + self.table_, + location, + location_offset, + location_length, Location ) } @@ -139,9 +140,10 @@ impl<'a> IndividualTable<'a> { row.into().0, 0, self.num_rows(), - self.table_.parents, - self.table_.parents_offset, - self.table_.parents_length, + self.table_, + parents, + parents_offset, + parents_length, IndividualId ) } diff --git a/src/metadata.rs b/src/metadata.rs index 0e211b1da..bc81bbf22 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -269,6 +269,15 @@ pub(crate) fn char_column_to_slice( Some(x) => x, None => return None, }; + debug_assert!(!column.is_null()); + debug_assert!(!column_offset.is_null()); + if column.is_null() { + return None; + } + if column_offset.is_null() { + return None; + } + // SAFETY: not null and best effort bounds check let start = unsafe { *column_offset.offset(row_isize) }; let stop = if (row as tsk_size_t) < num_rows { unsafe { *column_offset.offset(row_isize + 1) } diff --git a/src/migration_table.rs b/src/migration_table.rs index 45a0bba00..8951fc7db 100644 --- a/src/migration_table.rs +++ b/src/migration_table.rs @@ -98,7 +98,7 @@ impl<'a> MigrationTable<'a> { /// * `Some(position)` if `row` is valid. /// * `None` otherwise. pub fn left + Copy>(&'a self, row: M) -> Option { - unsafe_tsk_column_access_and_map_into!(row.into().0, 0, self.num_rows(), self.table_.left) + unsafe_tsk_column_access_and_map_into!(row.into().0, 0, self.num_rows(), self.table_, left) } /// Return the right coordinate for a given row. @@ -108,7 +108,7 @@ impl<'a> MigrationTable<'a> { /// * `Some(positions)` if `row` is valid. /// * `None` otherwise. pub fn right + Copy>(&'a self, row: M) -> Option { - unsafe_tsk_column_access_and_map_into!(row.into().0, 0, self.num_rows(), self.table_.right) + unsafe_tsk_column_access_and_map_into!(row.into().0, 0, self.num_rows(), self.table_, right) } /// Return the node for a given row. @@ -118,7 +118,7 @@ impl<'a> MigrationTable<'a> { /// * `Some(node)` if `row` is valid. /// * `None` otherwise. pub fn node + Copy>(&'a self, row: M) -> Option { - unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_.node, NodeId) + unsafe_tsk_column_access!(row.into().0, 0, self.num_rows(), self.table_, node, NodeId) } /// Return the source population for a given row. @@ -132,7 +132,8 @@ impl<'a> MigrationTable<'a> { row.into().0, 0, self.num_rows(), - self.table_.source, + self.table_, + source, PopulationId ) } @@ -148,7 +149,8 @@ impl<'a> MigrationTable<'a> { row.into().0, 0, self.num_rows(), - self.table_.dest, + self.table_, + dest, PopulationId ) } @@ -160,7 +162,7 @@ impl<'a> MigrationTable<'a> { /// * `Some(time)` if `row` is valid. /// * `None` otherwise. pub fn time + Copy>(&'a self, row: M) -> Option