Properly escaping scoped GDAL objects

[joa-quim]

Hi,
This is more a question on opinion if the GDAL crash that one obtains with the bellow example is due to bad usage or it reflects a true problem in GDAL.

If I take the last example in Quick Start and change it to write a in MEM dataset to look like this (I think I'm allowed to do it but not 100% sure)

AG.create(
    "",
    driver = AG.getdriver("MEM"),
    width=240,
    height=180,
    nbands=1,
    dtype=Float64
) do dataset
    AG.write!(dataset, Z, 1)
    AG.setgeotransform!(dataset, gt)
    AG.setproj!(dataset, crs)

   @show(dataset)
   #AG.getband(dataset, 1)
end

it prints

julia> include("test_gdal.jl")
dataset = GDAL Dataset (Driver: MEM/In Memory Raster)
File(s):

Dataset (width x height): 240 x 180 (pixels)
Number of raster bands: 1
  [GA_Update] Band 1 (Undefined): 240 x 180 (Float64)

NULL Dataset

So apparently it worked. But if I uncomment the #AG.getband(dataset, 1) line then GDAL crashes

Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
Exception: EXCEPTION_ACCESS_VIOLATION at 0x7ffcf0076bfc -- GDALGetRasterColorInterpretation at C:\programs\compa_libs\DLLs_VC14_64\gdal_w64.DLL (unknown line)

Now, it happens that I've already seen this crashing message when trying to feed in GDAL an MEM driver written in C (from within GMT). At the time I discussed it a bit with Even Rouault in the GDAL mailing list but could not come out with a simple example that he could reproduce.

Whose fault do you think this is?

[visr]

But if I uncomment the #AG.getband(dataset, 1) line then GDAL crashes

This works fine, it shows the band and then shows 1.

    band = AG.getband(dataset, 1)
    @show band
    1
end

If you have the band as the last line, you are returning it from the do block that will destroy the dataset upon completion. So you end up with a rasterband with no dataset, leading to issues. The crash happens because the REPL will call show on the result, which apparently crashes here:

ArchGDAL.jl/src/base/display.jl

Line 57 in c1d9faf

color = getname(getcolorinterp(rasterband))

It would be good to read this part of the docs, where most of this is explained:
https://yeesian.com/ArchGDAL.jl/dev/memory/

[visr]

Although as far as I see this usage can only really be an error, it's probably not a great user experience to get a segfault that kills the session.

Reopening because perhaps with a few is null checks we can error here before the segfault happens, which I believe is already being done elsewhere in the code.

[yeesian]

(I think I'm allowed to do it but not 100% sure)

Yeah it looks like reasonable, and I'm sorry the design of the package so far has not avoided this class of bugs -- hopefully we can arrive at a design which doesn't require people to have to know about #171 (comment) in order to program safely.

I don't have any solutions for it yet, but it's good to keep this bug open until we do.

[joa-quim]

OK, no problem. I changed the example to not run it in a do block and then it worked. It was just a such damn coincidence (or not?) that I managed to reproduce a GDAL crash in the exact same place that I had before when trying to send an in-mem dataset to gdalwarp, but this time from C.

[yeesian]

@visr, @evetion, @rafaqz, any other watchers who're interested: thinking out loud here, might it make sense for us to return an interactive copy of the rasterband/etc if it's on the last line of a do-block?

• I think that's be the most intuitive/safe behavior (that still allows for people to return values from the do-block). The downside is if people use the do-block without expecting objects to be created unnecessarily -- but I think such classes of developers are more likely to profile their code and identify it.
• An alternative that I've considered (to enforce safe behavior) is to not allow for values to be returned from the do-block. The downside is that it's a fairly common use-case/expectation, and can be annoying to work around.

[rafaqz]

Do you mean check if the return value is a band, and if so dont close it?

Not allowing return values would break a lot if things.

[yeesian]

Do you mean check if the return value is a band, and if so dont close it?

Yeah, with two important distinctions (in parenthesis):

1. check if the return value [will be finalized],
2. and if so [create a deep copy of it to be returned]