MDArray::read_into_slice(): make it robust to too small buffer

[rouault]

I'm not totally sure if it is the right approach because I changed the signature from accepting a slice to requiring a Vec. Perhaps the right fix would be to remove the "pub" qualifier of that method as it is rather low level, or tag it as unsafe

[lnicola]

This isn't quite right. Vec::capacity is the allocated space, while Vec::len is the number of elements that actually exist. By checking the capacity without setting the length, the last elements will be inaccessible.

But anyway, I think we can leave it as a slice (buffer: &mut [T]) and check the length like if buffer.len() < pixels. Slices know their length.

[rouault]

But anyway, I think we can leave it as a slice (buffer: &mut [T]) and check the length like if buffer.len() < pixels. Slices know their length.

That wouldn't work because the read_as() method uses Vec::with_capacity(pixels); before calling read_into_slice() on it. Such vector has .len() == 0. read_as() calls set_len(pixels) afterwards. I've just tested that calling set_len() before calling read_into_slice() works however. Line 230 of mdarray.rs refers to #examples-18 to justify this pattern, but I suspect this is now https://doc.rust-lang.org/std/vec/struct.Vec.html#examples-21 . Not obvious to me if it would be fundamentally wrong to set_len() before read_into_slice()

[lnicola]

Good point, I missed that call. Just filed an alternative implementation that splits the function in two instead, hope you don't mind.

Not obvious to me if it would be fundamentally wrong to set_len() before read_into_slice()

Yes. It would probably be "harmless" in this case, but for something like:

    let mut vec: Vec<String> = Vec::with_capacity(200);
    unsafe { vec.set_len(vec.capacity()) };

    for i in 0..vec.capacity() as isize {
        if i == 50 {
            panic!();
        }
        unsafe { std::ptr::write(vec.as_mut_ptr().offset(i), String::new()); }
    }

the panic works just like a C++ exception (it is one, actually), so the Drop implementation (destructor) of vec will try to destroy 200 elements. If we call vec.set_len at the end, this is "just" a memory leak.

[rouault]

closing as superseded per https://github.com/georust/gdal/pull/480