georust / gdal

Rust bindings for GDAL
https://crates.io/crates/gdal
MIT License
339 stars 92 forks source link

`RasterBand::read_block` is unsound #485

Closed lnicola closed 7 months ago

lnicola commented 7 months ago

https://github.com/georust/gdal/blob/d279c9f4cd5f81469d206a3de296afaa72bd533a/src/raster/rasterband.rs#L534

We don't validate that T matches the band data type, so there's nothing preventing the user from using a smaller type.

I've used something like:

#[derive(Debug)]
pub enum TypedBlock {
    U16(Array2<u16>),
    I16(Array2<i16>),
    F32(Array2<f32>),
    // ...
}

pub trait RasterBandExt {
    fn read_typed_block(&self, x: usize, y: usize) -> gdal::errors::Result<TypedBlock>;
}

impl<'d> RasterBandExt for RasterBand<'d> {
    fn read_typed_block(&self, x: usize, y: usize) -> gdal::errors::Result<TypedBlock> {
        match self.band_type() {
            GdalDataType::UInt16 => {
                let buf = self.read_block::<u16>((x, y))?;
                Ok(TypedBlock::U16(buf))
            }
            GdalDataType::Int16 => {
                let buf = self.read_block::<i16>((x, y))?;
                Ok(TypedBlock::I16(buf))
            }
            GdalDataType::Float32 => {
                let buf = self.read_block::<f32>((x, y))?;
                Ok(TypedBlock::F32(buf))
            }
            // ...
        }
    }
}

before, but I don't think it's the best possible abstraction here.

jdroenner commented 7 months ago

yes we should check the type here. It is not the same as the normal read raster method where gdal will cast the type.