Possible memory leak in _get_att() when reading a string attribute

[ckhroulev]

Unlike nc_get_att_text, the NetCDF C API function nc_get_att_string allocates storage for the attribute. The user is expected to free this storage using nc_free_string. (Unfortunately the documentation of nc_get_att_string is wrong.)

The implementation of _get_att calls nc_get_att_string, but does not seem to free this storage.

Please let me know if I'm just confused about this (and feel free to close this issue).

Thanks!

[jswhit]

Don't know if this is a problem or not. Haven't had any reports of memory leaks - have you?

[jswhit]

I tried a simple test that writes a string attribute and then reads it back in a loop, and indeed it does seem to leak. I've created pull request #496 to add an nc_free_string call after reading the attribute, as you suggest. I've also added an nc_free_string call after reading a VLEN string variable. The C lib also has nc_free_vlen, which servers a similar purpose for VLEN variable reads. However, when I add a call to nc_free_vlen after reading VLEN data, tst_vlen.py fails, so I've left this call commented out.

[dopplershift]

attn: @WardF (regarding C-API docs)

[ckhroulev]

@jswhit : I did not know if this is a problem (and no, I haven't seen any memory leak reports)-- I was just reading _netCDF4.pyx and noticed that nc_free_string is not used.

Thanks for the fix in #496. I looked at the test failure you mention; please consider using the patch included below.

From a050ff5020a38363efb4b8fb6e303febfe3db72b Mon Sep 17 00:00:00 2001
From: Constantine Khroulev <ckhroulev@alaska.edu>
Date: Tue, 8 Dec 2015 09:41:05 -0900
Subject: [PATCH] Fix "regular vlen" case in Variable._get(...).

We need to manually copy data from nc_vlen_t structures to NumPy arrays.

In the code prior to this commit the internal storage of dataarr
is *replaced* with the array allocated by the NetCDF library, leaking
memory allocated when dataarr is created and making it impossible to
de-allocate vldata using nc_free_vlens().
---
 netCDF4/_netCDF4.pyx | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/netCDF4/_netCDF4.pyx b/netCDF4/_netCDF4.pyx
index c5cad63..2c82ac8 100644
--- a/netCDF4/_netCDF4.pyx
+++ b/netCDF4/_netCDF4.pyx
@@ -943,6 +943,8 @@ import_array()
 include "netCDF4.pxi"
 include "constants.pyx"

+from libc.string cimport memcpy
+
 # check for required version of netcdf-4 and hdf5.

 def _gethdf5libversion():
@@ -4364,15 +4366,15 @@ The default value of `mask` is `True`
                 for i from 0<=i<totelem:
                     arrlen  = vldata[i].len
                     dataarr = numpy.empty(arrlen, self.dtype)
-                    dataarr.data = <char *>vldata[i].p
+                    memcpy(<void*>dataarr.data, vldata[i].p, dataarr.nbytes)
                     data[i] = dataarr
                 # reshape the output array
                 data = numpy.reshape(data, shapeout)
                 # free vlen data internally allocated in netcdf C lib.
                 # (tst_vlen.py fails with this line uncommented)
-                #ierr = nc_free_vlens(totelem, vldata)
+                ierr = nc_free_vlens(totelem, vldata)
                 # free the pointer array.
-                #free(vldata)
+                free(vldata)
         free(startp)
         free(countp)
         free(stridep)
-- 
2.6.2

@dopplershift : Please see Unidata/netcdf-c#171 .

[ckhroulev]

Oh, by the way: I noticed that vlen string array attributes are not supported.

Now that I understand how to use nc_get_att_string I think this would be easy to support and am willing to implement it (and create a pull request). What do you think?

[jswhit]

Thanks - the pull request has been updated with your memcpy trick.

[jswhit]

Sure, having support for vlen string array attributes would be nice, although I can't think of a use case offhand.

[jswhit]

pull request #496 merged, closing issue now. Thanks again for flagging this.