Opened 13 years ago

Closed 13 years ago

#3313 closed defect (wontfix)

GRASS driver crash after MAPSET unset

Reported by: rblazek Owned by: warmerdam
Priority: normal Milestone:
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords:
Cc: rblazek, martinl

Description

The bug was first reported in qgis: http://trac.osgeo.org/qgis/ticket/1900

The problem is that, if MAPSET environment variable is unset by an application, the next call of IRasterIO crashes because it calls G_set_window which needs a MAPSET to find if MASK exists. There are othere similar calls.

I have written new function ResetMapset which is called before any access to data. Patch is attached, should I commit to svn?

Attachments (1)

grass57dataset.cpp.patch (2.5 KB ) - added by rblazek 13 years ago.
patch

Download all attachments as: .zip

Change History (5)

by rblazek, 13 years ago

Attachment: grass57dataset.cpp.patch added

patch

comment:1 by Even Rouault, 13 years ago

Cc: martinl added

CC'ing Martin Landa as he might be interested

comment:2 by Even Rouault, 13 years ago

Radim, I'm not very qualified when it comes to GRASS, but I guess you can commit it if you think it's reasonnably safe to apply after your testing. The integration of GRASS in any application seem to be rather fragile due to the use of those global environment variables.

I see that ResetMapset() could be called quite frequently as it is called from IRasterIO for example. Have you checked that it doesn't cause too many memory leaks ? I remember having tried to fix memory leaks in the driver in the past, but I just discovered that most of them were in the GRASS libs themselves.

comment:3 by rblazek, 13 years ago

You are right, Gsetenv() calls set_env() which allocates memory for the new value using G_store() but does not release memory allocated for previous value. Unfortunately even G_unsetenv() does not release the memory and there is no other access to the store where variables are stored. I have created http://trac.osgeo.org/grass/ticket/863

Looking into GRASS code, I also discovered the Gsetenv() is quite complex and it can consume a lot of time, it is bad to call it for each raster row (which is the case in qgis for example).

G_reset_mapsets() is fast but it does not release the memory. http://trac.osgeo.org/grass/ticket/864

G_add_mapset_to_search_path() allocates memory with G_store() which is not released by G_reset_mapsets().

There is another possibility to fix the bug, just setting G_suppress_masking() disables checking for MASK file and the MAPSET is not used. It should also speed up the driver. I am convinced that MASK is not used much and maybe GDAL should ignore it in any case (I remember one advanced GRASS user becoming mad because of forgotten MASK set). OTOH, GRASS lib can change in future and require MAPSET to be set for other purpose and if GDAL begins to ignore MASK now, it means that GDAL behaviour changes.

comment:4 by rblazek, 13 years ago

Resolution: wontfix
Status: newclosed

The patchx is bad as explained above. The solution would be to add to GRASS functions for switching between more environments as described here http://trac.osgeo.org/grass/ticket/863#comment:4 Setting G_suppress_masking() would also possible and not harmful I think.

The original problem in QGIS was solved by writing native GRASS driver.

Note: See TracTickets for help on using tickets.