georust / gdal

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

Debug-printing geometries segfaults #365

Open lnicola opened 1 year ago

lnicola commented 1 year ago

Something like:

fn main() -> Result<()> {
    let args = std::env::args().collect::<Vec<_>>();
    let point_ds = Dataset::open(&args[1])?;
    let mut layer = point_ds.layer(0)?;
    for feature in layer.features() {
        dbg!(feature);
    }
    Ok(())
}

crashes with:

[src/main.rs:17] feature = Feature {
    _defn: Defn {
        c_defn: 0x0000562e86c81c80,
    },
    c_feature: 0x0000562e86c85d60,
    geometry: [
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', ~/.cargo/registry/src/github.com-1ecc6299db9ec823/gdal-0.14.0/src/vector/geometry.rs:177:38
stack backtrace:
   0: rust_begin_unwind
             at /rustc/efd27454ae94386e11997b1158f60d3a79deeb7d/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/efd27454ae94386e11997b1158f60d3a79deeb7d/library/core/src/panicking.rs:64:14
   2: core::panicking::panic
             at /rustc/efd27454ae94386e11997b1158f60d3a79deeb7d/library/core/src/panicking.rs:114:5
   3: gdal::vector::geometry::Geometry::wkt
   4: <&T as core::fmt::Debug>::fmt
metasim commented 1 year ago

@lnicola Was going to replicate on MacOS. Can you share the input file?

lnicola commented 1 year ago

It happens with every input, like the sample from https://github.com/ngageoint/GeoPackage/blob/master/docs/examples/java/example.gpkg.

Tested with the crates.io version, though.

metasim commented 1 year ago

FWIW, here's what I get from master:

     Running `.../georust-gdal/target/debug/examples/segfault ./example.gpkg`
Warning 1: Database relies on the 'nga_contents_id' (http://ngageoint.github.io/GeoPackage/docs/extensions/contents-id.html) extension that should be implemented in order to read it safely, but is not currently. Some data may be missing while reading that database.
Warning 1: Database relies on the 'nga_feature_tile_link' (http://ngageoint.github.io/GeoPackage/docs/extensions/feature-tile-link.html) extension that should be implemented in order to read it safely, but is not currently. Some data may be missing while reading that database.
Warning 1: Layer point1 relies on the 'nga_geometry_index' (http://ngageoint.github.io/GeoPackage/docs/extensions/geometry-index.html) extension that should be implemented in order to read it safely, but is not currently. Some data may be missing while reading that layer.
[examples/segfault.rs:9] feature = Feature {
    _defn: Defn {
        c_defn: 0x000060000304afd0,
    },
    c_feature: 0x00006000030763f0,
    geometry: [
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/vector/geometry.rs:177:38
metasim commented 1 year ago

Not really the problem, but calling unwrap seems like a bad idea?

https://github.com/georust/gdal/blob/c7f88b56a855cfdff44267009f2ad1730cd03b14/src/vector/geometry.rs#L177

metasim commented 1 year ago

@lnicola I wonder if this (foreign_types) or something like it might be worth considering. I think we need some principled semantics around these GDAL pointers, Rust has all the type machinery we need, we just need to find good examples of how to do this properly or rely on a library that does it for us.

lnicola commented 1 year ago

Still haven't looked into it, but I think we handle layers in a better way, that could serve as inspiration.

metasim commented 1 year ago

I have foreign_types prototype here: https://github.com/georust/gdal/compare/master...metasim:gdal:prototype/foreign_types

Personally, once I converted a couple types, I really liked it (except the paucity of documentation), especially since it explicitly distinguishes owned vs. borrowed types. It's a very principled API. However, there would be several breaking changes.