Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raster MEM driver: disable opening a dataset with MEM::: syntax by default #10861

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

rouault
Copy link
Member

@rouault rouault commented Sep 22, 2024

    Starting with GDAL 3.10, opening a MEM dataset using the above syntax is no
    longer enabled by default for security reasons.
    If you want to allow it, define the ``GDAL_MEM_ENABLE_OPEN`` configuration
    option to ``YES``, or build GDAL with the ``GDAL_MEM_ENABLE_OPEN`` compilation
    definition.

    .. config:: GDAL_MEM_ENABLE_OPEN
       :choices: YES, NO
       :default: NO
       :since: 3.10

       Whether opening a MEM dataset with the ``MEM:::`` syntax is allowed.

…fault

```
    Starting with GDAL 3.10, opening a MEM dataset using the above syntax is no
    longer enabled by default for security reasons.
    If you want to allow it, define the ``GDAL_MEM_ENABLE_OPEN`` configuration
    option to ``YES``, or build GDAL with the ``GDAL_MEM_ENABLE_OPEN`` compilation
    definition.

    .. config:: GDAL_MEM_ENABLE_OPEN
       :choices: YES, NO
       :default: NO
       :since: 3.10

       Whether opening a MEM dataset with the ``MEM:::`` syntax is allowed.
```
@rouault rouault added this to the 3.10.0 milestone Sep 22, 2024
@mdsumner mdsumner mentioned this pull request Sep 23, 2024
@rouault
Copy link
Member Author

rouault commented Sep 23, 2024

@mdsumner Ah, so they are people using that outside of GDAL. Interesting.

An alternative to opening with "MEM:::DATAPOINTER=..." is to use the GDALCreate() function on the MEM driver, creating 0 bands.
And then use GDALAddBand(hDS, options) with options DATAPOINTER, PIXELOFFSET and LINEOFFSET.
And possibly using SetGeoTransform() / SetSpatialRef() to add additional properties.
Cf https://github.com/OSGeo/gdal/blob/master/gcore/rasterio.cpp#L1551-L1592
So instead of vapour::gdal_raster_data(mem(v, ....)) you'd rather offer a vapour::gdal_raster_from_mem(v, ....)

The unsafe part of the "MEM:::DATAPOINTER=..." open syntax is that it could be used in a hostile context as a tile name of a virtual mosaic format to causes crashes or worse.

@rouault
Copy link
Member Author

rouault commented Sep 23, 2024

@sgillies ok, I see rasterio/rasterio uses that at https://github.com/rasterio/rasterio/blob/ffe77ecc7bd0f92597e6ca700e0987f33ba8f0f9/rasterio/_io.pyx#L2256 .
Same suggestion as above for the full future proof solution. A quick workaround is to obviously use CPLSetThreadLocalConfigOption("GDAL_MEM_ENABLE_OPEN", "YES") / CPLSetThreadLocalConfigOption("GDAL_MEM_ENABLE_OPEN", NULL) around that line.

@mdsumner
Copy link
Contributor

mdsumner commented Sep 24, 2024

I think in R the people is me-only. :) Useful for exploring warper output on crafted matrices. Appreciate the heads up about how I should provide it.

The other case I saw it being used (slightly) outside of rasterio:

https://gist.github.com/vincentsarago/b00f50f8b66ab5ccbd4449f6a1bd8b71

@SpaceCyclist
Copy link

SpaceCyclist commented Feb 6, 2025

@mdsumner Ah, so they are people using that outside of GDAL. Interesting.

Hello, we are also using it in OrfeoToolBox (if changes can help, you can look here https://gitlab.orfeo-toolbox.org/orfeotoolbox/otb/-/merge_requests/1056 )
About the migration, users needs to also to be careful if they previously use a custom BANDOFFSET parameter. They need to report this shift between bands when using DATAPOINTER option that you can add with AddBand function.

Also I was wondering: now how do you open a dataset with the "GA_ReadOnly" or "GA_Update" ?

@rouault
Copy link
Member Author

rouault commented Feb 6, 2025

now how do you open a dataset with the "GA_ReadOnly" or "GA_Update" ?

The former for read-only scenarios, the later for read-update ones

@SpaceCyclist
Copy link

now how do you open a dataset with the "GA_ReadOnly" or "GA_Update" ?

The former for read-only scenarios, the later for read-update ones

Thank you, but that was not want I wanted to know, maybe my previous question is confuse.
Before your changes we could do:
MEMDataset* mds = GDALOpen("MEM::: ...", GA_ReadOnly) or MEMDataset* mds = GDALOpen("MEM::: ...", GA_Update).
This parameter is set here https://github.com/OSGeo/gdal/blob/master/frmts/mem/memdataset.cpp#L1188 . How do you set this parameter now that we use the GdalDriver->Create syntax ?

@rouault
Copy link
Member Author

rouault commented Feb 6, 2025

How do you set this parameter now that we use the GdalDriver->Create syntax ?

Ah I see. You can't. Datasets returned by Create() are assumed to be read-write. If you provide a read-only memory mapping, you should be careful not to call RasterIO(GF_Write, ...) or another method that can alter pixel values. That's admitedly an oversight

@SpaceCyclist
Copy link

Ok, thank you for the clarification! Indeed the MEMDataSet::Create set the eAccess to GA_Update by default, I miss this one.

Maybe the best way to do it in our code is to provide one read-only wrapper method which returns a const DataSet and one R/W method which returns a writable dataset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants