gdal.Open() and ogr.Open() GRASS data does not close projection files properly.

[cgsbob]

With the script below, I am able to demonstrate that gdal does not close the projection files after a grass raster file is closed (by setting raster = None).

Each call to gdal.Open() opens /home/bobm/usr/grass-6.4.svn/etc/datum.table four times, /home/bobm/usr/grass-6.4.svn/etc/datumtransform.table one time, and /home/bobm/usr/grass-6.4.svn/etc/ellipse.table three times and stay open even after setting raster = None.

I'm running grass and gdal from svn head on a Hardy Heron X86_64.

#=========================== opengrassrast.py ======================================
#!/usr/bin/env python

try:
    from osgeo import gdal
    from osgeo import osr
    from osgeo import ogr
except ImportError:
    import gdal
    import osr
    import ogr
    import sys, pydb

    c=0

    while c < 20:
        filepath = "/home/bobm/Data/Grass/peerproj/test1/cellhd/pleist_avg"
        raster =  gdal.Open(filepath)
        raster = None
        if c > int(sys.argv[1]):
            print "To see the open files type: lsof -p `ps -fe | grep python| grep opengrassrast.py | cut -d" " -f7`"
            pydb.debugger()

[Even Rouault]

I don't think this is related to the bindings, but to the driver itself

[cgsbob]

I also wanted to add that I am using Grass 6.4.svn revision 32980 and gdal revision 15172

[cgsbob]

ogr.Open() has the same problem, but it leaves a lot more projection files open. Each call to ogr.Open() opens /home/bobm/usr/grass-6.4.svn/etc/datum.table eight times, /home/bobm/usr/grass-6.4.svn/etc/datumtransform.table two time, and /home/bobm/usr/grass-6.4.svn/etc/ellipse.table six times and stay open even after setting vector = None.

[Even Rouault]

The problem is in GRASS itself, not in the GDAL-GRASS driver. A few file descriptors were not closed. With the patches, I can GDALOpen/GDALClose in an endless loop.

I'm attaching 2 patches :

• 1 for memory leaks in the GDAL-GRASS driver. I'm a bit reluctant to apply it directly, because with grass 6.4, G_find_file2 return must apparently be freed. Was it always the case ? Is it a recent change ? Markus should confirm.
• 1 to close the files opened by GRASS, and that fixes many memory leaks. GRASS would need a Valgrind'ing session I guess...

I'm adding Markus in CC so that it can review the attached patch to GRASS.

[Even Rouault]

Patch to the GDAL-GRASS driver

[Even Rouault]

Patch to r33046 of GRASS

[Even Rouault]

Markus, in fact, by looking more closely at find_file() in lib/gis/find_file.c, there are code paths that lead to returning an already-allocated string (line 68, with an evil cast), and other to a newly-allocated string (line 83). That should be clarified/fixed. So the G_free(ret) in my 'patch_to_gdal_grass_driver_for2537.patch' is incorrect for the moment.

[Even Rouault]

As well as the similar change in lib/gis/range.c in 'grass_patch_for_gdal2537.patch'.

cgsbob, you can probably safely only apply lib/proj/datum.c and in lib/proj/ellipse.c for the moment.

[Markus Neteler]

On Sun, Aug 24, 2008 at 10:06 PM, Glynn Clements wrote:

Someone else needs to be reminded that the solution to memory leaks in GRASS libraries is not to use those libraries from persistent applications. The GRASS libraries exist for GRASS, and are designed for GRASS; whether or not they are useful for anything else isn't really an issue.

OTOH, I've committed a fix for the descriptor leak in lib/proj/datum.c; that actually matters.

-- Glynn Clements

Fixed in GRASS in r33052 (trunk) and r33053 (6.4.svn).

[Even Rouault]

Markus, thanks, but as far as I can see, only the leak of FILE* in lib/proj/datum.c have been applied. There's still a leak of FILE* in lib/proj/ellipse.c that was addressed by my patch.

[Even Rouault]

I've applied in r15209 my patch, without the part related to G_find_file2() where it is not clear if the return value must be freed or not.

[cgsbob]

Trying to find out why Tyler Mitchell's gdalogr_catalog (you can find it at ​http://code.google.com/p/spatialguru/wiki/SpatialDataCataloguingScript) was crashing when it was processing Grass data led me to finding this bug. With the latest patches, I've noticed that the projection files do not stay open, but the Grass vector files themselves seem to stay open (each vector has a coor and hist file opened multiple times).

[Markus Neteler]

More comments from Glynn here: ​http://lists.osgeo.org/pipermail/grass-dev/2008-August/039623.html

[glynn]

Replying to cgsbob:

Trying to find out why Tyler Mitchell's gdalogr_catalog (you can find it at ​http://code.google.com/p/spatialguru/wiki/SpatialDataCataloguingScript) was crashing when it was processing Grass data led me to finding this bug. With the latest patches, I've noticed that the projection files do not stay open, but the Grass vector files themselves seem to stay open (each vector has a coor and hist file opened multiple times).

The OGR driver never calls Vect_close() on poMap.

Although it's not clear from the documentation, I would guess that this should be done within the destructor for OGRGRASSDataSource.

[Even Rouault]

I've commited in r15235 the call to Vect_close() in the destructor for OGRGRASSDataSource. I've verified that it closes the filed descriptors that were opened with the previous version, however there are still huge memory leaks (worse than for the GDAL part I think).

See :

==32276== ERROR SUMMARY: 13 errors from 5 contexts (suppressed: 159 from 1)
==32276== malloc/free: in use at exit: 140,837 bytes in 2,746 blocks.
==32276== malloc/free: 5,366 allocs, 2,620 frees, 464,954 bytes allocated.
==32276== For counts of detected errors, rerun with: -v
==32276== searching for pointers to 2,746 not-freed blocks.
==32276== checked 5,339,516 bytes.
==32276==
==32276==
==32276== 17 bytes in 1 blocks are definitely lost in loss record 7 of 37
==32276==    at 0x4022765: malloc (vg_replace_malloc.c:149)
==32276==    by 0x610FC47: vasprintf (in /lib/tls/i686/cmov/libc-2.6.1.so)
==32276==    by 0x7312D02: G_vasprintf (asprintf.c:61)
==32276==    by 0x7312D39: G_asprintf (asprintf.c:122)
==32276==    by 0x73825D7: GPJ_grass_to_osr (convert.c:200)
==32276==    by 0x7381E57: GPJ_grass_to_wkt (convert.c:59)
==32276==    by 0x5245CE5: OGRGRASSLayer::OGRGRASSLayer(int, Map_info*) (ogrgrasslayer.cpp:204)
==32276==    by 0x5242A5E: OGRGRASSDataSource::Open(char const*, int, int, int) (ogrgrassdatasource.cpp:210)
==32276==    by 0x5242147: OGRGRASSDriver::Open(char const*, int) (ogrgrassdriver.cpp:61)
==32276==    by 0x4B05859: OGRSFDriverRegistrar::Open(char const*, int, OGRSFDriver**) (ogrsfdriverregistrar.cpp:205)
==32276==    by 0x804BA1F: main (ogrinfo.cpp:173)
==32276==
==32276==
==32276== 156 (36 direct, 120 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 37
==32276==    at 0x4022765: malloc (vg_replace_malloc.c:149)
==32276==    by 0x61939B2: (within /lib/tls/i686/cmov/libc-2.6.1.so)
==32276==    by 0x6194208: __nss_database_lookup (in /lib/tls/i686/cmov/libc-2.6.1.so)
==32276==    by 0x7599FDB: ???
==32276==    by 0x759B11C: ???
==32276==    by 0x6140EEB: getpwuid_r (in /lib/tls/i686/cmov/libc-2.6.1.so)
==32276==    by 0x614089D: getpwuid (in /lib/tls/i686/cmov/libc-2.6.1.so)
==32276==    by 0x734E9EC: G_whoami (whoami.c:70)
==32276==    by 0x73E1CCC: Vect__init_head (init_head.c:44)
==32276==    by 0x73ECC31: Vect__open_old (open.c:145)
==32276==    by 0x73ED837: Vect_open_old (open.c:412)
==32276==    by 0x5242969: OGRGRASSDataSource::Open(char const*, int, int, int) (ogrgrassdatasource.cpp:190)
==32276==
==32276==
==32276== 72 bytes in 1 blocks are possibly lost in loss record 19 of 37
==32276==    at 0x4022765: malloc (vg_replace_malloc.c:149)
==32276==    by 0x7446B2E: dig_alloc_line (struct_alloc.c:114)
==32276==    by 0x7440D75: dig_Rd_P_line (plus_struct.c:166)
==32276==    by 0x743DEF3: dig_load_plus (plus.c:302)
==32276==    by 0x73EE3B3: Vect_open_topo (open.c:748)
==32276==    by 0x73ED0F8: Vect__open_old (open.c:228)
==32276==    by 0x73ED837: Vect_open_old (open.c:412)
==32276==    by 0x5242969: OGRGRASSDataSource::Open(char const*, int, int, int) (ogrgrassdatasource.cpp:190)
==32276==    by 0x5242147: OGRGRASSDriver::Open(char const*, int) (ogrgrassdriver.cpp:61)
==32276==    by 0x4B05859: OGRSFDriverRegistrar::Open(char const*, int, OGRSFDriver**) (ogrsfdriverregistrar.cpp:205)
==32276==    by 0x804BA1F: main (ogrinfo.cpp:173)
==32276==
==32276==
==32276== 476 bytes in 1 blocks are possibly lost in loss record 23 of 37
==32276==    at 0x4022765: malloc (vg_replace_malloc.c:149)
==32276==    by 0x744D049: RTreeNewNode (node.c:49)
==32276==    by 0x744C22E: RTreeNewIndex (index.c:29)
==32276==    by 0x7444DF4: dig_spidx_init (spindex.c:36)
==32276==    by 0x743D964: dig_init_plus (plus.c:94)
==32276==    by 0x73ECC42: Vect__open_old (open.c:146)
==32276==    by 0x73ED837: Vect_open_old (open.c:412)
==32276==    by 0x5242969: OGRGRASSDataSource::Open(char const*, int, int, int) (ogrgrassdatasource.cpp:190)
==32276==    by 0x5242147: OGRGRASSDriver::Open(char const*, int) (ogrgrassdriver.cpp:61)
==32276==    by 0x4B05859: OGRSFDriverRegistrar::Open(char const*, int, OGRSFDriver**) (ogrsfdriverregistrar.cpp:205)
==32276==    by 0x804BA1F: main (ogrinfo.cpp:173)
==32276==
==32276==
==32276== 3,332 bytes in 7 blocks are definitely lost in loss record 29 of 37
==32276==    at 0x4022765: malloc (vg_replace_malloc.c:149)
==32276==    by 0x744D049: RTreeNewNode (node.c:49)
==32276==    by 0x744C22E: RTreeNewIndex (index.c:29)
==32276==    by 0x7444E04: dig_spidx_init (spindex.c:37)
==32276==    by 0x743D964: dig_init_plus (plus.c:94)
==32276==    by 0x73ECC42: Vect__open_old (open.c:146)
==32276==    by 0x73ED837: Vect_open_old (open.c:412)
==32276==    by 0x5242969: OGRGRASSDataSource::Open(char const*, int, int, int) (ogrgrassdatasource.cpp:190)
==32276==    by 0x5242147: OGRGRASSDriver::Open(char const*, int) (ogrgrassdriver.cpp:61)
==32276==    by 0x4B05859: OGRSFDriverRegistrar::Open(char const*, int, OGRSFDriver**) (ogrsfdriverregistrar.cpp:205)
==32276==    by 0x804BA1F: main (ogrinfo.cpp:173)
==32276==
==32276==
==32276== 11,695 (1,903 direct, 9,792 indirect) bytes in 17 blocks are definitely lost in loss record 31 of 37
==32276==    at 0x4022765: malloc (vg_replace_malloc.c:149)
==32276==    by 0x7310CF1: G_malloc (alloc.c:41)
==32276==    by 0x743A5E9: dig_cidx_init (cindex.c:35)
==32276==    by 0x743D96F: dig_init_plus (plus.c:95)
==32276==    by 0x73ECC42: Vect__open_old (open.c:146)
==32276==    by 0x73ED837: Vect_open_old (open.c:412)
==32276==    by 0x5242969: OGRGRASSDataSource::Open(char const*, int, int, int) (ogrgrassdatasource.cpp:190)
==32276==    by 0x5242147: OGRGRASSDriver::Open(char const*, int) (ogrgrassdriver.cpp:61)
==32276==    by 0x4B05859: OGRSFDriverRegistrar::Open(char const*, int, OGRSFDriver**) (ogrsfdriverregistrar.cpp:205)
==32276==    by 0x804BA1F: main (ogrinfo.cpp:173)
==32276==
==32276==
==32276== 108,952 (5,260 direct, 103,692 indirect) bytes in 4 blocks are definitely lost in loss record 32 of 37
==32276==    at 0x4022765: malloc (vg_replace_malloc.c:149)
==32276==    by 0x40227EF: realloc (vg_replace_malloc.c:306)
==32276==    by 0x7446AD3: dig_alloc_nodes (struct_alloc.c:99)
==32276==    by 0x743DE38: dig_load_plus (plus.c:290)
==32276==    by 0x73EE3B3: Vect_open_topo (open.c:748)
==32276==    by 0x73ED0F8: Vect__open_old (open.c:228)
==32276==    by 0x73ED837: Vect_open_old (open.c:412)
==32276==    by 0x5242969: OGRGRASSDataSource::Open(char const*, int, int, int) (ogrgrassdatasource.cpp:190)
==32276==    by 0x5242147: OGRGRASSDriver::Open(char const*, int) (ogrgrassdriver.cpp:61)
==32276==    by 0x4B05859: OGRSFDriverRegistrar::Open(char const*, int, OGRSFDriver**) (ogrsfdriverregistrar.cpp:205)
==32276==    by 0x804BA1F: main (ogrinfo.cpp:173)
==32276==
==32276== LEAK SUMMARY:
==32276==    definitely lost: 10,548 bytes in 30 blocks.
==32276==    indirectly lost: 113,604 bytes in 2,608 blocks.
==32276==      possibly lost: 548 bytes in 2 blocks.
==32276==    still reachable: 16,137 bytes in 106 blocks.
==32276==         suppressed: 0 bytes in 0 blocks.
==32276== Reachable blocks (those to which a pointer was found) are not shown.
==32276== To see them, rerun with: --leak-check=full --show-reachable=yes

In that state of the GRASS librairies, it can be used one time per process, and not in a loop of thousands of iterations.

I close the bug, as the remaining issues are no more in the GDAL/OGR part.

[Markus Neteler]

For the record - a related bug was recently fixed: ​http://trac.osgeo.org/grass/changeset/34395 and ​http://trac.osgeo.org/grass/changeset/34396

[Markus Neteler]

More precisely: r34411 and r34412 eliminate the leaks regarding "file" and "errbuf".