From 5098894ffde76620db2798fba900287aaf518166 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Tue, 11 Jun 2019 04:09:27 -0700 Subject: Fixed a memory issue where unfreed shared attribute dataspace memory tripped an assert in our memory sanity checks. Fixes HDFFV-10774. --- src/H5Oattr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/H5Oattr.c b/src/H5Oattr.c index 0a7c4bf..f9c5fcf 100644 --- a/src/H5Oattr.c +++ b/src/H5Oattr.c @@ -200,7 +200,7 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags, * What's actually shared, though, is only the extent. */ if(NULL == (attr->shared->ds = H5FL_CALLOC(H5S_t))) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed") + HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed") /* Decode attribute's dataspace extent */ if((extent = (H5S_extent_t *)(H5O_MSG_SDSPACE->decode)(f, open_oh, @@ -253,6 +253,8 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags, done: if(NULL == ret_value) if(attr) { + if(attr->shared->ds) + attr->shared->ds = H5FL_FREE(H5S_t, attr->shared->ds); if(attr->shared) { /* Free any dynamically allocated items */ if(H5A__free(attr) < 0) -- cgit v0.12 From c4ee9ef7c05ce79e4503c1e5b3edfe8f912de60e Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Tue, 11 Jun 2019 04:09:27 -0700 Subject: Fixed a memory issue where unfreed shared attribute dataspace memory tripped an assert in our memory sanity checks. Fixes HDFFV-10774. --- src/H5Adense.c | 10 +++------- src/H5Aint.c | 17 +++++++++++------ src/H5Oattr.c | 13 ++++--------- src/H5S.c | 10 +++++++--- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/H5Adense.c b/src/H5Adense.c index 81e0dc5..ceac7d9 100644 --- a/src/H5Adense.c +++ b/src/H5Adense.c @@ -325,14 +325,10 @@ H5A__dense_fnd_cb(const H5A_t *attr, hbool_t *took_ownership, void *_user_attr) */ if(*user_attr != NULL) { H5A_t *old_attr = *user_attr; - if(old_attr->shared) { - /* Free any dynamically allocated items */ - if(H5A__free(old_attr) < 0) - HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release attribute info") - /* Destroy shared attribute struct */ - old_attr->shared = H5FL_FREE(H5A_shared_t, old_attr->shared); - } /* end if */ + /* Free any dynamically allocated items */ + if(H5A__free(old_attr) < 0) + HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release attribute info") old_attr = H5FL_FREE(H5A_t, old_attr); } /* end if */ diff --git a/src/H5Aint.c b/src/H5Aint.c index 6162401..2240657 100644 --- a/src/H5Aint.c +++ b/src/H5Aint.c @@ -1113,24 +1113,32 @@ H5A__free(H5A_t *attr) HDassert(attr); - /* Free dynamically allocated items */ + if(!attr->shared) + HGOTO_DONE(SUCCEED) + + /* Free dynamically allocated items. + * When possible, keep trying to shut things down (via HDONE_ERROR). + */ if(attr->shared->name) { H5MM_xfree(attr->shared->name); attr->shared->name = NULL; } if(attr->shared->dt) { if(H5T_close_real(attr->shared->dt) < 0) - HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release datatype info") + HDONE_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release datatype info") attr->shared->dt = NULL; } if(attr->shared->ds) { if(H5S_close(attr->shared->ds) < 0) - HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release dataspace info") + HDONE_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release dataspace info") attr->shared->ds = NULL; } if(attr->shared->data) attr->shared->data = H5FL_BLK_FREE(attr_buf, attr->shared->data); + /* Destroy shared attribute struct */ + attr->shared = H5FL_FREE(H5A_shared_t, attr->shared); + done: FUNC_LEAVE_NOAPI(ret_value) } /* end H5A__free() */ @@ -1199,9 +1207,6 @@ H5A__close(H5A_t *attr) /* Free dynamically allocated items */ if(H5A__free(attr) < 0) HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release attribute info") - - /* Destroy shared attribute struct */ - attr->shared = H5FL_FREE(H5A_shared_t, attr->shared); } /* end if */ else { /* There are other references to the shared part of the attribute. diff --git a/src/H5Oattr.c b/src/H5Oattr.c index 0a7c4bf..653c23a 100644 --- a/src/H5Oattr.c +++ b/src/H5Oattr.c @@ -200,7 +200,7 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags, * What's actually shared, though, is only the extent. */ if(NULL == (attr->shared->ds = H5FL_CALLOC(H5S_t))) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed") + HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed") /* Decode attribute's dataspace extent */ if((extent = (H5S_extent_t *)(H5O_MSG_SDSPACE->decode)(f, open_oh, @@ -253,14 +253,9 @@ H5O_attr_decode(H5F_t *f, H5O_t *open_oh, unsigned H5_ATTR_UNUSED mesg_flags, done: if(NULL == ret_value) if(attr) { - if(attr->shared) { - /* Free any dynamically allocated items */ - if(H5A__free(attr) < 0) - HDONE_ERROR(H5E_ATTR, H5E_CANTRELEASE, NULL, "can't release attribute info") - - /* Destroy shared attribute struct */ - attr->shared = H5FL_FREE(H5A_shared_t, attr->shared); - } /* end if */ + /* Free any dynamically allocated items */ + if(H5A__free(attr) < 0) + HDONE_ERROR(H5E_ATTR, H5E_CANTRELEASE, NULL, "can't release attribute info") attr = H5FL_FREE(H5A_t, attr); } /* end if */ diff --git a/src/H5S.c b/src/H5S.c index 3926b5f..e50985f 100644 --- a/src/H5S.c +++ b/src/H5S.c @@ -448,10 +448,14 @@ H5S_close(H5S_t *ds) if(H5S__extent_release(&ds->extent) < 0) HGOTO_ERROR(H5E_DATASPACE, H5E_CANTRELEASE, FAIL, "unable to release dataspace extent") - /* Release the main structure */ - ds = H5FL_FREE(H5S_t, ds); - done: + /* Release the main structure. + * Always do this to ensure that we don't leak memory when calling this + * function on partially constructed dataspaces (which will fail one or + * both of the above calls) + */ + H5FL_FREE(H5S_t, ds); + FUNC_LEAVE_NOAPI(ret_value) } /* end H5S_close() */ -- cgit v0.12 From 0cf52525d5b45cc7cf6f89267da76b46ffa75f90 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Tue, 11 Jun 2019 14:14:13 -0700 Subject: H5A__free() was renamed to H5A__shared_free() and now requires attr->shared to not be NULL. --- src/H5Adense.c | 5 +++-- src/H5Aint.c | 26 +++++++++++++------------- src/H5Apkg.h | 2 +- src/H5Oattr.c | 5 +++-- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/H5Adense.c b/src/H5Adense.c index ceac7d9..bddfe31 100644 --- a/src/H5Adense.c +++ b/src/H5Adense.c @@ -327,8 +327,9 @@ H5A__dense_fnd_cb(const H5A_t *attr, hbool_t *took_ownership, void *_user_attr) H5A_t *old_attr = *user_attr; /* Free any dynamically allocated items */ - if(H5A__free(old_attr) < 0) - HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release attribute info") + if(old_attr->shared) + if(H5A__shared_free(old_attr) < 0) + HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release attribute info") old_attr = H5FL_FREE(H5A_t, old_attr); } /* end if */ diff --git a/src/H5Aint.c b/src/H5Aint.c index 2240657..2126444 100644 --- a/src/H5Aint.c +++ b/src/H5Aint.c @@ -1092,29 +1092,29 @@ done: /*------------------------------------------------------------------------- - * Function: H5A__free + * Function: H5A__shared_free * - * Purpose: Frees all memory associated with an attribute, but does not - * free the H5A_t structure (which should be done in H5T_close). + * Purpose: Cleans up the shared attribute data. This will free + * the attribute's shared structure as well. + * + * attr and attr->shared must not be NULL * * Return: SUCCEED/FAIL * - * Programmer: Quincey Koziol - * Monday, November 15, 2004 + * Programmer: Quincey Koziol + * Monday, November 15, 2004 * *------------------------------------------------------------------------- */ herr_t -H5A__free(H5A_t *attr) +H5A__shared_free(H5A_t *attr) { herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_PACKAGE HDassert(attr); - - if(!attr->shared) - HGOTO_DONE(SUCCEED) + HDassert(attr->shared); /* Free dynamically allocated items. * When possible, keep trying to shut things down (via HDONE_ERROR). @@ -1139,9 +1139,8 @@ H5A__free(H5A_t *attr) /* Destroy shared attribute struct */ attr->shared = H5FL_FREE(H5A_shared_t, attr->shared); -done: FUNC_LEAVE_NOAPI(ret_value) -} /* end H5A__free() */ +} /* end H5A__shared_free() */ /*------------------------------------------------------------------------- @@ -1205,8 +1204,9 @@ H5A__close(H5A_t *attr) /* Reference count can be 0. It only happens when H5A__create fails. */ if(attr->shared->nrefs <= 1) { /* Free dynamically allocated items */ - if(H5A__free(attr) < 0) - HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release attribute info") + if(attr->shared) + if(H5A__shared_free(attr) < 0) + HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release attribute info") } /* end if */ else { /* There are other references to the shared part of the attribute. diff --git a/src/H5Apkg.h b/src/H5Apkg.h index 91061cd..f3870c0 100644 --- a/src/H5Apkg.h +++ b/src/H5Apkg.h @@ -196,7 +196,7 @@ H5_DLL H5A_t *H5A__copy(H5A_t *new_attr, const H5A_t *old_attr); H5_DLL hid_t H5A__get_type(H5A_t *attr); H5_DLL herr_t H5A__get_info(const H5A_t *attr, H5A_info_t *ainfo); H5_DLL hid_t H5A__get_create_plist(H5A_t* attr); -H5_DLL herr_t H5A__free(H5A_t *attr); +H5_DLL herr_t H5A__shared_free(H5A_t *attr); H5_DLL herr_t H5A__close(H5A_t *attr); H5_DLL herr_t H5A__close_cb(H5VL_object_t *attr_vol_obj); H5_DLL htri_t H5A__get_ainfo(H5F_t *f, H5O_t *oh, H5O_ainfo_t *ainfo); diff --git a/src/H5Oattr.c b/src/H5Oattr.c index 653c23a..f685a00c 100644 --- a/src/H5Oattr.c +++ b/src/H5Oattr.c @@ -254,8 +254,9 @@ done: if(NULL == ret_value) if(attr) { /* Free any dynamically allocated items */ - if(H5A__free(attr) < 0) - HDONE_ERROR(H5E_ATTR, H5E_CANTRELEASE, NULL, "can't release attribute info") + if(attr->shared) + if(H5A__shared_free(attr) < 0) + HDONE_ERROR(H5E_ATTR, H5E_CANTRELEASE, NULL, "can't release attribute info") attr = H5FL_FREE(H5A_t, attr); } /* end if */ -- cgit v0.12