Writing NC_VLEN attribute of NC_STRINGs can result in garbled values

[krisfed]

Hi all,

We are seeing a difference in writing behavior between variables and attributes of the "NC_VLEN of NC_STRINGs" type. Under some conditions, writing NC_VLEN attribute of NC_STRINGs can result in garbage values, while writing a similar variable under the same conditions seems to work fine.

As far as I can tell it has something to do with attributes needing NC_VLEN data to still be in memory by the time of the nc_close() call.

Here is some simplistic code that reproduces the issue. It uses the same code for writing a variable and for writing an attribute. The raw NC_VLEN data should be getting cleaned up by the time of nc_close() call (it is in a different scope):

#include <iostream>
#include <vector>
#include <string>
#include "netcdf.h"

void checkErrorCode(int status, const char* message){
    if (status != NC_NOERR){
        std::cout << "Error code: " << status << " from " << message << std::endl;
        std::cout << nc_strerror(status) << std::endl << std::endl;
    }
}


void writeVariable(const int dimlength, int ncid, nc_type vlen_typeID)
{
    // Setup data
    std::unique_ptr<nc_vlen_t[]> data(new nc_vlen_t[dimlength]);
    
    const int totalNumStrings = 13;
    std::string stringsInVLEN[totalNumStrings] = {"banana", "kiwi", "apple", "grape", "apple", "cherry", "lemon", "melon", "watermelon", "strawberry", "peach", "raspberry", "pineapple"};
    const char* charPointers[totalNumStrings];
    
    for (int i=0; i<totalNumStrings; i++){
        charPointers[i] = stringsInVLEN[i].c_str();
    }
    
    // create six variable length arrays of strings
    int stringIndex = 0;
    
    const int first_size = 2;
    data[0].len = first_size;
    data[0].p = charPointers+stringIndex;
    stringIndex += first_size;
    
    const int second_size = 1;
    data[1].len = second_size;
    data[1].p = charPointers+stringIndex;
    stringIndex += second_size;
    
    const int third_size = 5;
    data[2].len = third_size;
    data[2].p = charPointers+stringIndex;
    stringIndex += third_size;
    
    const int fourth_size = 3;
    data[3].len = fourth_size;
    data[3].p = charPointers+stringIndex;
    stringIndex += fourth_size;
    
    const int fifth_size = 0;
    data[4].len = fifth_size;
    data[4].p = charPointers+stringIndex;
    stringIndex += fifth_size;
    
    const int sixth_size = 2;
    data[5].len = sixth_size;
    data[5].p = charPointers+stringIndex;
    stringIndex += sixth_size;
    
        
    // Write vlen variable named Fruits
    
    // Define dimension
    int dimid;
    int retval = nc_def_dim(ncid, "D1", dimlength, &dimid);
    checkErrorCode(retval, "nc_def_dim");
    
    // Define variable
    int varid;
    retval = nc_def_var(ncid, "Fruits", vlen_typeID, 1, &dimid, &varid);
    checkErrorCode(retval, "nc_def_var");
    
    // Write variable
    retval = nc_put_var(ncid, varid, data.get());
    checkErrorCode(retval, "nc_put_var");
    
}

void writeAttribute(const int attlength, int ncid, nc_type vlen_typeID)
{
    // Setup data
    std::unique_ptr<nc_vlen_t[]> data(new nc_vlen_t[attlength]);
    
    const int totalNumStrings = 13;
    std::string stringsInVLEN[totalNumStrings] = {"banana", "kiwi", "apple", "grape", "apple", "cherry", "lemon", "melon", "watermelon", "strawberry", "peach", "raspberry", "pineapple"};
    const char* charPointers[totalNumStrings];
    
    for (int i=0; i<totalNumStrings; i++){
        charPointers[i] = stringsInVLEN[i].c_str();
    }
    
    // create six variable length arrays of strings
    int stringIndex = 0;
    
    const int first_size = 2;
    data[0].len = first_size;
    data[0].p = charPointers+stringIndex;
    stringIndex += first_size;
    
    const int second_size = 1;
    data[1].len = second_size;
    data[1].p = charPointers+stringIndex;
    stringIndex += second_size;
    
    const int third_size = 5;
    data[2].len = third_size;
    data[2].p = charPointers+stringIndex;
    stringIndex += third_size;
    
    const int fourth_size = 3;
    data[3].len = fourth_size;
    data[3].p = charPointers+stringIndex;
    stringIndex += fourth_size;
    
    const int fifth_size = 0;
    data[4].len = fifth_size;
    data[4].p = charPointers+stringIndex;
    stringIndex += fifth_size;
    
    const int sixth_size = 2;
    data[5].len = sixth_size;
    data[5].p = charPointers+stringIndex;
    stringIndex += sixth_size;
    
        
    // Write vlen attribute named Fruits
    int retval = nc_put_att(ncid, NC_GLOBAL, "Fruit", vlen_typeID, attlength, data.get());
    checkErrorCode(retval, "nc_put_att");
    
}

int main(int argc, const char * argv[]) {
    
    const int attlength = 6;

    // Open file
    int ncid;
    int retval;
    
    retval = nc_create("vlen_of_strings_att.nc", NC_NETCDF4, &ncid);
    checkErrorCode(retval, "nc_create");
    
    // Define vlen type named MY_VLEN_STRING
    nc_type vlen_typeID;
    retval = nc_def_vlen(ncid, "MY_VLEN_STRING", NC_STRING, &vlen_typeID);
    checkErrorCode(retval, "nc_def_vlen");
    
    // write variable - will be able to read it back fine
    writeVariable(attlength, ncid, vlen_typeID);
    
    // write attribute - will read back garbled values
    writeAttribute(attlength, ncid, vlen_typeID);
    
    retval = nc_close(ncid);
    checkErrorCode(retval, "nc_close");
    
    return retval;
}

Here is the results of running it. The error codes for all operations seem to be NC_NOERR (so it is a silent failure). You can see that ncdump reads the variable fine, but the attribute's data is garbled. We see the same when we try to read the attribute data with nc_get_att().

$ ./a.out 
$ ncdump vlen_of_strings_att.nc 
netcdf vlen_of_strings_att {
types:
  string(*) MY_VLEN_STRING ;
dimensions:
	D1 = 6 ;
variables:
	MY_VLEN_STRING Fruits(D1) ;

// global attributes:
		MY_VLEN_STRING :Fruit = {"banana", "\n?\002\001"}, {""}, 
    {"????\177", "apple", "????\177", "", "#?\002\001"}, 
    {"?\005??\177", "", "?\005??\177"}, {}, {"????\177", "?\005??\177"} ;
data:

 Fruits = {"banana", "kiwi"}, {"apple"}, 
    {"grape", "apple", "cherry", "lemon", "melon"}, 
    {"watermelon", "strawberry", "peach"}, {}, {"raspberry", "pineapple"} ;
}

We are using netcdf 4.8.1, and the above code was run on a mac (but we are seeing the same issue on other platforms).

Does this look like a bug? Is it a known issue?

[DennisHeimbigner]

Sadly the VLEN code needs substantial refactoring.
I just have not had time to revisit it.
As for needing to stay in memory, where did you see that. I think
it is supposed to copy.

[krisfed]

Thanks for taking a look Dennis!

I just thought it might have to do with staying in memory because I did not see this issue in cases when nc_close() is in the same scope as the NC_VLEN data, or if data is created with "new", but "delete" is not called, and in similar cases where NC_VLEN data should still be in memory before the nc_close() call. So that was my best guess at what might be the reason behind this issue.

It also sounds like it could be a little related to the other NC_VLEN issue I reported earlier: #2041 , where there were errors/crashes on nc_close() when writing a "nested" NC_VLEN attribute. I know NC_VLEN of NC_STRINGs is kind of similar to a nested NC_VLEN (as NC_STRING is an NC_VLEN of NC_CHARs). So in both cases there is something special about nc_close() call when writing (nested-ish) NC_VLEN attributes.

[krisfed]

Wanted to note that with writing some very large NC_VLENs of NC_STRINGs attributes, we also saw a crash on nc_close(). Additional similarity with that previously reported issue.

[DennisHeimbigner]

I did some more experimentation with this. I think your guess is mostly correct.
The attribute data is probably copied internally but it does not do a deep
copy. I think it does a shallow copy of the top level. This would explain why we do not
see this for vlens of the int types or enums. I would expect it for vlens of opaque, string,
some compounds and vlens of vlens.
I can hack together a fix; it is really important to you?

[krisfed]

It would be really nice to have a fix in the next netcdf release, if possible!

[DennisHeimbigner]

I am working on the fix (actually the more general problem of reclaiming VLENS).
I am mostly done, except I have one very difficult problem I am trying to solve.
Attached is the proposed PR report that describes the changes.
vlenfix.txt

[krisfed]

Thank you very much for working on this, Dennis!

If nc_free_vlen(s) are deprecated in favor of ncaux_reclaim_data, and if netcdf_aux.h file is being touched anyway, could "typeid" argument name for ncaux_end_compound be renamed to make it easier to compile with C++ (as I mentioned on #2078)?

[DennisHeimbigner]

Oops I forgot about typeid. I will make sure it is removed.