From bc2bd03a466909e874220e3f08e6abe79a5dbe9b Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Wed, 10 Jun 2009 14:22:09 -0500 Subject: [svn-r17026] Purpose: fix bug 1593 Description: When using H5T_copy on committed datatypes that are already open, H5T_copy would properly use the already existing shared struct, but would still deep copy all of the fields in that struct. This would cause memory leaks, and in the case of a compound containing a vlen (or reference), the change in size would cause the size of the resulting type to be set to an incorrect value. Changed H5T_copy to properly avoid deep copies when using a reopened shared struct. Tested: jam, linew, smirom (h5committest), purify on jam --- release_docs/RELEASE.txt | 3 +- src/H5T.c | 200 +++++++++++++++++++++++----------------------- test/dtypes.c | 204 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 308 insertions(+), 99 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 6043aae..bdc3f15 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -98,7 +98,8 @@ Bug Fixes since HDF5-1.8.3 Library ------- - - None + - Fixed an issue with committed compound datatypes containing a vlen. Also + fixed memory leaks involving committed datatypes. NAF - 2009/06/10 - 1593 Parallel Library ---------------- diff --git a/src/H5T.c b/src/H5T.c index 0345e5f..200d5c1 100644 --- a/src/H5T.c +++ b/src/H5T.c @@ -3161,118 +3161,122 @@ H5T_copy(const H5T_t *old_dt, H5T_copy_t method) break; } /* end switch */ - /* Copy parent information, if we aren't sharing an already opened committed datatype */ - if(NULL == reopened_fo && old_dt->shared->parent) - new_dt->shared->parent = H5T_copy(old_dt->shared->parent, method); + /* Update fields in the new struct, if we aren't sharing an already opened + * committed datatype */ + if(!reopened_fo) { + /* Copy parent information */ + if(old_dt->shared->parent) + new_dt->shared->parent = H5T_copy(old_dt->shared->parent, method); + + switch(new_dt->shared->type) { + case H5T_COMPOUND: + { + int accum_change = 0; /* Amount of change in the offset of the fields */ - switch(new_dt->shared->type) { - case H5T_COMPOUND: - { - int accum_change = 0; /* Amount of change in the offset of the fields */ + /* + * Copy all member fields to new type, then overwrite the + * name and type fields of each new member with copied values. + * That is, H5T_copy() is a deep copy. + */ + /* Only malloc if space has been allocated for members - NAF */ + if(new_dt->shared->u.compnd.nalloc > 0) { + new_dt->shared->u.compnd.memb = H5MM_malloc(new_dt->shared->u.compnd.nalloc * + sizeof(H5T_cmemb_t)); + if (NULL==new_dt->shared->u.compnd.memb) + HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed"); + + HDmemcpy(new_dt->shared->u.compnd.memb, old_dt->shared->u.compnd.memb, + new_dt->shared->u.compnd.nmembs * sizeof(H5T_cmemb_t)); + } /* end if */ - /* - * Copy all member fields to new type, then overwrite the - * name and type fields of each new member with copied values. - * That is, H5T_copy() is a deep copy. - */ - /* Only malloc if space has been allocated for members - NAF */ - if(new_dt->shared->u.compnd.nalloc > 0) { - new_dt->shared->u.compnd.memb = H5MM_malloc(new_dt->shared->u.compnd.nalloc * - sizeof(H5T_cmemb_t)); - if (NULL==new_dt->shared->u.compnd.memb) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed"); + for(i = 0; i < new_dt->shared->u.compnd.nmembs; i++) { + unsigned j; + int old_match; - HDmemcpy(new_dt->shared->u.compnd.memb, old_dt->shared->u.compnd.memb, - new_dt->shared->u.compnd.nmembs * sizeof(H5T_cmemb_t)); - } /* end if */ + s = new_dt->shared->u.compnd.memb[i].name; + new_dt->shared->u.compnd.memb[i].name = H5MM_xstrdup(s); + tmp = H5T_copy (old_dt->shared->u.compnd.memb[i].type, method); + new_dt->shared->u.compnd.memb[i].type = tmp; + HDassert(tmp != NULL); - for(i = 0; i < new_dt->shared->u.compnd.nmembs; i++) { - unsigned j; - int old_match; + /* Apply the accumulated size change to the offset of the field */ + new_dt->shared->u.compnd.memb[i].offset += accum_change; + + if(old_dt->shared->u.compnd.sorted != H5T_SORT_VALUE) { + for(old_match = -1, j = 0; j < old_dt->shared->u.compnd.nmembs; j++) { + if(!HDstrcmp(new_dt->shared->u.compnd.memb[i].name, old_dt->shared->u.compnd.memb[j].name)) { + old_match = j; + break; + } /* end if */ + } /* end for */ + + /* check if we couldn't find a match */ + if(old_match < 0) + HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCOPY, NULL, "fields in datatype corrupted"); + } /* end if */ + else + old_match = i; - s = new_dt->shared->u.compnd.memb[i].name; - new_dt->shared->u.compnd.memb[i].name = H5MM_xstrdup(s); - tmp = H5T_copy (old_dt->shared->u.compnd.memb[i].type, method); - new_dt->shared->u.compnd.memb[i].type = tmp; - HDassert(tmp != NULL); + /* If the field changed size, add that change to the accumulated size change */ + if(new_dt->shared->u.compnd.memb[i].type->shared->size != old_dt->shared->u.compnd.memb[old_match].type->shared->size) { + /* Adjust the size of the member */ + new_dt->shared->u.compnd.memb[i].size = (old_dt->shared->u.compnd.memb[old_match].size*tmp->shared->size)/old_dt->shared->u.compnd.memb[old_match].type->shared->size; - /* Apply the accumulated size change to the offset of the field */ - new_dt->shared->u.compnd.memb[i].offset += accum_change; + accum_change += (new_dt->shared->u.compnd.memb[i].type->shared->size - old_dt->shared->u.compnd.memb[old_match].type->shared->size); + } /* end if */ + } /* end for */ - if(old_dt->shared->u.compnd.sorted != H5T_SORT_VALUE) { - for(old_match = -1, j = 0; j < old_dt->shared->u.compnd.nmembs; j++) { - if(!HDstrcmp(new_dt->shared->u.compnd.memb[i].name, old_dt->shared->u.compnd.memb[j].name)) { - old_match = j; - break; - } /* end if */ - } /* end for */ + /* Apply the accumulated size change to the size of the compound struct */ + new_dt->shared->size += accum_change; - /* check if we couldn't find a match */ - if(old_match < 0) - HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCOPY, NULL, "fields in datatype corrupted"); - } /* end if */ - else - old_match = i; + } + break; - /* If the field changed size, add that change to the accumulated size change */ - if(new_dt->shared->u.compnd.memb[i].type->shared->size != old_dt->shared->u.compnd.memb[old_match].type->shared->size) { - /* Adjust the size of the member */ - new_dt->shared->u.compnd.memb[i].size = (old_dt->shared->u.compnd.memb[old_match].size*tmp->shared->size)/old_dt->shared->u.compnd.memb[old_match].type->shared->size; + case H5T_ENUM: + /* + * Copy all member fields to new type, then overwrite the name fields + * of each new member with copied values. That is, H5T_copy() is a + * deep copy. + */ + new_dt->shared->u.enumer.name = H5MM_malloc(new_dt->shared->u.enumer.nalloc * + sizeof(char*)); + new_dt->shared->u.enumer.value = H5MM_malloc(new_dt->shared->u.enumer.nalloc * + new_dt->shared->size); + if(NULL == new_dt->shared->u.enumer.value) + HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed"); + HDmemcpy(new_dt->shared->u.enumer.value, old_dt->shared->u.enumer.value, + new_dt->shared->u.enumer.nmembs * new_dt->shared->size); + for(i = 0; i < new_dt->shared->u.enumer.nmembs; i++) { + s = old_dt->shared->u.enumer.name[i]; + new_dt->shared->u.enumer.name[i] = H5MM_xstrdup(s); + } /* end for */ + break; - accum_change += (new_dt->shared->u.compnd.memb[i].type->shared->size - old_dt->shared->u.compnd.memb[old_match].type->shared->size); + case H5T_VLEN: + case H5T_REFERENCE: + if(method == H5T_COPY_TRANSIENT || method == H5T_COPY_REOPEN) { + /* H5T_copy converts any type into a memory type */ + if(H5T_set_loc(new_dt, NULL, H5T_LOC_MEMORY) < 0) + HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "invalid datatype location"); } /* end if */ - } /* end for */ - - /* Apply the accumulated size change to the size of the compound struct */ - new_dt->shared->size += accum_change; - - } - break; - - case H5T_ENUM: - /* - * Copy all member fields to new type, then overwrite the name fields - * of each new member with copied values. That is, H5T_copy() is a - * deep copy. - */ - new_dt->shared->u.enumer.name = H5MM_malloc(new_dt->shared->u.enumer.nalloc * - sizeof(char*)); - new_dt->shared->u.enumer.value = H5MM_malloc(new_dt->shared->u.enumer.nalloc * - new_dt->shared->size); - if(NULL == new_dt->shared->u.enumer.value) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed"); - HDmemcpy(new_dt->shared->u.enumer.value, old_dt->shared->u.enumer.value, - new_dt->shared->u.enumer.nmembs * new_dt->shared->size); - for(i = 0; i < new_dt->shared->u.enumer.nmembs; i++) { - s = old_dt->shared->u.enumer.name[i]; - new_dt->shared->u.enumer.name[i] = H5MM_xstrdup(s); - } /* end for */ - break; - - case H5T_VLEN: - case H5T_REFERENCE: - if(method == H5T_COPY_TRANSIENT || method == H5T_COPY_REOPEN) { - /* H5T_copy converts any type into a memory type */ - if(H5T_set_loc(new_dt, NULL, H5T_LOC_MEMORY) < 0) - HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "invalid datatype location"); - } /* end if */ - break; + break; - case H5T_OPAQUE: - /* - * Copy the tag name. - */ - new_dt->shared->u.opaque.tag = H5MM_xstrdup(new_dt->shared->u.opaque.tag); - break; + case H5T_OPAQUE: + /* + * Copy the tag name. + */ + new_dt->shared->u.opaque.tag = H5MM_xstrdup(new_dt->shared->u.opaque.tag); + break; - case H5T_ARRAY: - /* Re-compute the array's size, in case it's base type changed size */ - new_dt->shared->size=new_dt->shared->u.array.nelem*new_dt->shared->parent->shared->size; - break; + case H5T_ARRAY: + /* Re-compute the array's size, in case it's base type changed size */ + new_dt->shared->size=new_dt->shared->u.array.nelem*new_dt->shared->parent->shared->size; + break; - default: - break; - } /* end switch */ + default: + break; + } /* end switch */ + } /* end if */ /* Set the cached location & name path if the original type was a named * type and the new type is also named. diff --git a/test/dtypes.c b/test/dtypes.c index 21ad578..f35e68a 100644 --- a/test/dtypes.c +++ b/test/dtypes.c @@ -5806,6 +5806,209 @@ error: /*------------------------------------------------------------------------- + * Function: test_named_indirect_reopen + * + * Purpose: Tests that open named datatypes can be reopened indirectly + * through H5Dget_type without causing problems. + * + * Return: Success: 0 + * + * Failure: number of errors + * + * Programmer: Neil Fortner + * Thursday, June 4, 2009 + * + * Modifications: + * + *------------------------------------------------------------------------- + */ +static int +test_named_indirect_reopen(hid_t fapl) +{ + hid_t file=-1, type=-1, reopened_type=-1, strtype=-1, dset=-1, space=-1; + static hsize_t dims[1] = {3}; + size_t dt_size; + int enum_value; + const char *tag = "opaque_tag"; + char *tag_ret = NULL; + char filename[1024]; + + TESTING("indirectly reopening committed datatypes"); + + /* Create file, dataspace */ + h5_fixname(FILENAME[1], fapl, filename, sizeof filename); + if ((file=H5Fcreate (filename, H5F_ACC_TRUNC, H5P_DEFAULT, fapl)) < 0) TEST_ERROR + if ((space = H5Screate_simple (1, dims, dims)) < 0) TEST_ERROR + + /* + * Compound + */ + + /* Create compound type */ + if((strtype = H5Tcopy(H5T_C_S1)) < 0) TEST_ERROR + if(H5Tset_size(strtype, H5T_VARIABLE) < 0) TEST_ERROR + if((type = H5Tcreate(H5T_COMPOUND, sizeof(char *))) < 0) TEST_ERROR + if(H5Tinsert(type, "vlstr", 0, strtype) < 0) TEST_ERROR + if(H5Tclose(strtype) < 0) TEST_ERROR + + /* Get size of compound type */ + if((dt_size = H5Tget_size(type)) == 0) TEST_ERROR + + /* Commit compound type and verify the size doesn't change */ + if(H5Tcommit2(file, "cmpd_type", type, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT) < 0) TEST_ERROR + if(dt_size != H5Tget_size(type)) TEST_ERROR + + /* Create dataset with compound type */ + if((dset = H5Dcreate2(file, "cmpd_dset", type, space, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0) TEST_ERROR + + /* Indirectly reopen type and verify that the size doesn't change */ + if((reopened_type = H5Dget_type(dset)) < 0) TEST_ERROR + if(dt_size != H5Tget_size(reopened_type)) TEST_ERROR + + /* Close types and dataset */ + if(H5Tclose(type) < 0) TEST_ERROR + if(H5Tclose(reopened_type) < 0) TEST_ERROR + if(H5Dclose(dset) < 0) TEST_ERROR + + /* + * Enum + */ + + /* Create enum type */ + if((type = H5Tenum_create(H5T_NATIVE_INT)) < 0) TEST_ERROR + enum_value = 0; + if(H5Tenum_insert(type, "val1", &enum_value) < 0) TEST_ERROR + enum_value = 1; + if(H5Tenum_insert(type, "val2", &enum_value) < 0) TEST_ERROR + + /* Get size of enum type */ + if((dt_size = H5Tget_size(type)) == 0) TEST_ERROR + + /* Commit enum type and verify the size doesn't change */ + if(H5Tcommit2(file, "enum_type", type, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT) < 0) TEST_ERROR + if(dt_size != H5Tget_size(type)) TEST_ERROR + + /* Create dataset with enum type */ + if((dset = H5Dcreate2(file, "enum_dset", type, space, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0) TEST_ERROR + + /* Indirectly reopen type and verify that the size doesn't change */ + if((reopened_type = H5Dget_type(dset)) < 0) TEST_ERROR + if(dt_size != H5Tget_size(reopened_type)) TEST_ERROR + + /* Close types and dataset */ + if(H5Tclose(type) < 0) TEST_ERROR + if(H5Tclose(reopened_type) < 0) TEST_ERROR + if(H5Dclose(dset) < 0) TEST_ERROR + + /* + * Vlen + */ + + /* Create vlen type */ + if((type = H5Tvlen_create(H5T_NATIVE_INT)) < 0) TEST_ERROR + + /* Get size of vlen type */ + if((dt_size = H5Tget_size(type)) == 0) TEST_ERROR + + /* Commit vlen type and verify the size doesn't change */ + if(H5Tcommit2(file, "vlen_type", type, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT) < 0) TEST_ERROR + if(dt_size != H5Tget_size(type)) TEST_ERROR + + /* Create dataset with vlen type */ + if((dset = H5Dcreate2(file, "vlen_dset", type, space, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0) TEST_ERROR + + /* Indirectly reopen type and verify that the size doesn't change */ + if((reopened_type = H5Dget_type(dset)) < 0) TEST_ERROR + if(dt_size != H5Tget_size(reopened_type)) TEST_ERROR + + /* Close types and dataset */ + if(H5Tclose(type) < 0) TEST_ERROR + if(H5Tclose(reopened_type) < 0) TEST_ERROR + if(H5Dclose(dset) < 0) TEST_ERROR + + /* + * Opaque + */ + + /* Create opaque type */ + if((type = H5Tcreate(H5T_OPAQUE, 13)) < 0) TEST_ERROR + if(H5Tset_tag(type, tag) < 0) TEST_ERROR + + /* Get size of opaque type */ + if((dt_size = H5Tget_size(type)) == 0) TEST_ERROR + + /* Commit opaque type and verify the size and tag don't change */ + if(H5Tcommit2(file, "opaque_type", type, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT) < 0) TEST_ERROR + if(dt_size != H5Tget_size(type)) TEST_ERROR + if(NULL == (tag_ret = H5Tget_tag(type))) TEST_ERROR + if(HDstrcmp(tag, tag_ret)) TEST_ERROR + HDfree(tag_ret); + tag_ret = NULL; + + /* Create dataset with opaque type */ + if((dset = H5Dcreate2(file, "opaque_dset", type, space, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0) TEST_ERROR + + /* Indirectly reopen type and verify that the size and tag don't change */ + if((reopened_type = H5Dget_type(dset)) < 0) TEST_ERROR + if(dt_size != H5Tget_size(reopened_type)) TEST_ERROR + if(NULL == (tag_ret = H5Tget_tag(type))) TEST_ERROR + if(HDstrcmp(tag, tag_ret)) TEST_ERROR + HDfree(tag_ret); + tag_ret = NULL; + + /* Close types and dataset */ + if(H5Tclose(type) < 0) TEST_ERROR + if(H5Tclose(reopened_type) < 0) TEST_ERROR + if(H5Dclose(dset) < 0) TEST_ERROR + + /* + * Array + */ + + /* Create array type */ + if((type = H5Tarray_create2(H5T_NATIVE_INT, 1, dims)) < 0) TEST_ERROR + + /* Get size of array type */ + if((dt_size = H5Tget_size(type)) == 0) TEST_ERROR + + /* Commit array type and verify the size doesn't change */ + if(H5Tcommit2(file, "array_type", type, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT) < 0) TEST_ERROR + if(dt_size != H5Tget_size(type)) TEST_ERROR + + /* Create dataset with array type */ + if((dset = H5Dcreate2(file, "array_dset", type, space, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0) TEST_ERROR + + /* Indirectly reopen type and verify that the size doesn't change */ + if((reopened_type = H5Dget_type(dset)) < 0) TEST_ERROR + if(dt_size != H5Tget_size(reopened_type)) TEST_ERROR + + /* Close types and dataset */ + if(H5Tclose(type) < 0) TEST_ERROR + if(H5Tclose(reopened_type) < 0) TEST_ERROR + if(H5Dclose(dset) < 0) TEST_ERROR + + /* Close file and dataspace */ + if(H5Sclose(space) < 0) TEST_ERROR + if(H5Fclose(file) < 0) TEST_ERROR + PASSED(); + return 0; + +error: + H5E_BEGIN_TRY { + H5Tclose(type); + H5Tclose(strtype); + H5Tclose(reopened_type); + H5Sclose(space); + H5Dclose(dset); + H5Fclose(file); + } H5E_END_TRY; + if(tag_ret) + HDfree(tag_ret); + return 1; +} /* end test_named_indirect_reopen() */ + + +/*------------------------------------------------------------------------- * Function: test_deprec * * Purpose: Tests deprecated API routines for datatypes. @@ -5978,6 +6181,7 @@ main(void) nerrors += test_encode(); nerrors += test_latest(); nerrors += test_int_float_except(); + nerrors += test_named_indirect_reopen(fapl); #ifndef H5_NO_DEPRECATED_SYMBOLS nerrors += test_deprec(fapl); #endif /* H5_NO_DEPRECATED_SYMBOLS */ -- cgit v0.12