georust / gdal

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

`ExtendedDataType` improperly implements `Clone` #449

Closed metasim closed 9 months ago

metasim commented 9 months ago

ExtenedDataType is defined thus:

https://github.com/georust/gdal/blob/f638618d7b2212eee04d852ce4365c7c6da038e4/src/raster/mdarray.rs#L613-L617

Due to the derive'd Clone, you can get undefined behavior (UB) in the following way (results in SIGSEGV):

#[test]
fn clone_is_ub() {
    let fixture = "/vsizip/fixtures/byte_no_cf.zarr.zip";

    let dataset_options = DatasetOptions {
        open_flags: GdalOpenFlags::GDAL_OF_MULTIDIM_RASTER,
        allowed_drivers: None,
        open_options: None,
        sibling_files: None,
    };
    let dataset = Dataset::open_ex(fixture, dataset_options).unwrap();

    let root_group = dataset.root_group().unwrap();

    let md_array = root_group
        .open_md_array("byte_no_cf", CslStringList::new())
        .unwrap();

    let dt_clone = {
        let datatype = md_array.datatype();
        datatype.clone()
        // Fetched `ExtenedDataType` gets dropped
    };

    assert_eq!(dt_clone.class(), ExtendedDataTypeClass::Numeric);
    assert_eq!(dt_clone.numeric_datatype(), GDALDataType::GDT_Byte);
    assert_eq!(dt_clone.name(), "");
}

Arguably, PartialEq and Eq should not be derive either, but it's not UB.

metasim commented 9 months ago

I did some additional research into GDALAttributeGetDataType, and I'm not seeing anything indicating a clone-like operation is supported by the C API. Furthermore, while the C++ API implements the equality operator, I can't find a corresponding C API version (but perhaps it's elsewhere in the API). Given all that, my suggested fix to this is changing #[derive(Clone, Debug, PartialEq, Eq)] to #[derive(Debug)]. No tests fail after doing so.

lnicola commented 9 months ago

Yup, that matches my understanding of the GDAL API.