Multiple unsound problems in bcf crate

[lwz23]

rust-htslib/src/bcf/header.rs

Line 288 in da6278f

unsafe { slice::from_raw_parts(self.inner().samples, self.sample_count() as usize) };

Hello, thank you for your contribution in thin project. Currently, I'm scnning rust project in github, and I notice the following code.

pub struct HeaderView {
    pub inner: *mut htslib::bcf_hdr_t,
}

fn inner(&self) -> htslib::bcf_hdr_t {
        unsafe { *self.inner }
    }

 pub fn samples(&self) -> Vec<&[u8]> {
        let names =
            unsafe { slice::from_raw_parts(self.inner().samples, self.sample_count() as usize) };
        names
            .iter()
            .map(|name| unsafe { ffi::CStr::from_ptr(*name).to_bytes() })
            .collect()
    }

Since samples is a pub function, and inner is also a pub field, bcf is also a pub mod. So I guess this might mean that the user can manipulate the value of inner directly, and if inner is set to an eg. null pointer, it might cause UB.

[lwz23]

After some effort, I managed to get the project up and running, and I think I should be able to confirm that there is indeed an unsound problem.
here is my Poc:
Cargo.toml:

[package]
name = "lwz"
version = "0.1.0"
authors = ["lwz"]
edition = "2018"

[dependencies]
rust-htslib = "*"

main.rs:

extern crate rust_htslib;

use std::ptr;

use rust_htslib::bcf::header::HeaderView;


fn main() {
   let a=HeaderView::new(ptr::null_mut());
   let _=a.samples();
}

result:

lwz@LAPTOP-F68E79VH:/mnt/e/Github/lwz$ cargo run
   Compiling hts-sys v2.2.0
   Compiling rust-htslib v0.49.0
   Compiling lwz v0.1.0 (/mnt/e/Github/lwz)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 10.26s
     Running `target/debug/lwz`
Segmentation fault (core dumped)

[lwz23]

maybe same problem for Record's impl group like:

rust-htslib/src/bam/record.rs

Line 382 in da6278f

unsafe { slice::from_raw_parts_mut(self.inner.data, self.inner().l_data as usize) };

but I didn't test it, maybe we can also take a look at it ;)

[lwz23]

same problem for

rust-htslib/src/bcf/header.rs

Line 420 in da6278f

(*(*self.inner).id[htslib::BCF_DT_ID as usize].offset(*id as isize)).key,

rust-htslib/src/bcf/header.rs

Line 447 in da6278f

(*(*self.inner).id[htslib::BCF_DT_SAMPLE as usize].offset(*id as isize)).key,

rust-htslib/src/bcf/header.rs

Line 477 in da6278f

let rec = unsafe { &(**(*self.inner).hrec.offset(i as isize)) };

If there is no extern usage for HeaderView, maybe we should mark it as private, at least for its field should be marked as private. And add !null_ptr check in new method

[lwz23]

Same for Recode struct

rust-htslib/src/bcf/record.rs

Line 210 in da6278f

pub fn inner(&self) -> &htslib::bcf1_t {

rust-htslib/src/bcf/record.rs

Line 220 in da6278f

pub fn inner_mut(&mut self) -> &mut htslib::bcf1_t {

[lwz23]

ping?

[lwz23]

same for

rust-htslib/src/bam/mod.rs

Line 1369 in da6278f

inner: *mut htslib::bam_hdr_t,

alghough this time the field is not pub, user can still pass null pointer to inner with the new method

rust-htslib/src/bam/mod.rs

Line 1405 in da6278f

pub fn new(inner: *mut htslib::bam_hdr_t) -> Self {

[lwz23]

maybe same problem for

rust-htslib/src/bam/record.rs

Line 136 in da6278f

pub fn from_inner(from: *mut htslib::bam1_t) -> Self {

user can pass a null pointer to this function.