From 1ef8577a4aaaa4120a819ff36747fdca99abdc26 Mon Sep 17 00:00:00 2001 From: Vailin Choi Date: Wed, 23 Aug 2017 16:23:09 -0500 Subject: Modifications based on comments from pull request review (1) Remove unnecessary asserts (2) Add code to insert bad offset values to the test file in gen_bad_offset.c --- src/H5Glink.c | 21 ++++++++++++++------- src/H5Gnode.c | 43 ++++++++----------------------------------- src/H5Gstab.c | 6 ++---- src/H5Oefl.c | 3 ++- test/bad_offset.h5 | Bin 3312 -> 3312 bytes test/gen_bad_offset.c | 41 ++++++++++++++++++++++++++++++++++++++--- test/tmisc.c | 2 +- 7 files changed, 65 insertions(+), 51 deletions(-) diff --git a/src/H5Glink.c b/src/H5Glink.c index 77b69cb..e199f89 100644 --- a/src/H5Glink.c +++ b/src/H5Glink.c @@ -224,6 +224,7 @@ herr_t H5G__ent_to_link(H5O_link_t *lnk, const H5HL_t *heap, const H5G_entry_t *ent, const char *name) { + hbool_t dup_soft = FALSE; /* xstrdup the symbolic link name or not */ herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_PACKAGE @@ -238,19 +239,21 @@ H5G__ent_to_link(H5O_link_t *lnk, const H5HL_t *heap, lnk->cset = H5F_DEFAULT_CSET; lnk->corder = 0; lnk->corder_valid = FALSE; /* Creation order not valid for this link */ - lnk->name = H5MM_xstrdup(name); - HDassert(lnk->name); + if((lnk->name = H5MM_xstrdup(name)) == NULL) + HGOTO_ERROR(H5E_LINK, H5E_CANTGET, FAIL, "unable to duplicate link name") /* Object is a symbolic or hard link */ if(ent->type == H5G_CACHED_SLINK) { const char *s; /* Pointer to link value */ if((s = (const char *)H5HL_offset_into(heap, ent->cache.slink.lval_offset)) == NULL) - HGOTO_ERROR(H5E_LINK, H5E_CANTGET, FAIL, "unable to get link name") - HDassert(s); + HGOTO_ERROR(H5E_LINK, H5E_CANTGET, FAIL, "unable to get symbolic link name") /* Copy the link value */ - lnk->u.soft.name = H5MM_xstrdup(s); + if((lnk->u.soft.name = H5MM_xstrdup(s)) == NULL) + HGOTO_ERROR(H5E_LINK, H5E_CANTGET, FAIL, "unable to duplicate symbolic link name") + + dup_soft = TRUE; /* Set link type */ lnk->type = H5L_TYPE_SOFT; @@ -264,8 +267,12 @@ H5G__ent_to_link(H5O_link_t *lnk, const H5HL_t *heap, } /* end else */ done: - if(ret_value < 0 && lnk->name) - H5MM_xfree(lnk->name); + if(ret_value < 0) { + if(lnk->name) + H5MM_xfree(lnk->name); + if(ent->type == H5G_CACHED_SLINK && dup_soft) + H5MM_xfree(lnk->u.soft.name); + } FUNC_LEAVE_NOAPI(ret_value) } /* end H5G__ent_to_link() */ diff --git a/src/H5Gnode.c b/src/H5Gnode.c index 4ddcf53..76e2c4b 100644 --- a/src/H5Gnode.c +++ b/src/H5Gnode.c @@ -35,7 +35,7 @@ /***********/ /* Headers */ /***********/ -#include "H5private.h" /* Generic Functionsi */ +#include "H5private.h" /* Generic Functions */ #include "H5ACprivate.h" /* Metadata cache */ #include "H5Eprivate.h" /* Error handling */ #include "H5Fprivate.h" /* File access */ @@ -396,7 +396,6 @@ H5G_node_cmp2(void *_lt_key, void *_udata, void *_rt_key) H5G_node_key_t *lt_key = (H5G_node_key_t *) _lt_key; H5G_node_key_t *rt_key = (H5G_node_key_t *) _rt_key; const char *s1, *s2; - const char *base; /* Base of heap */ int ret_value = SUCCEED; /* Return value */ FUNC_ENTER_NOAPI_NOINIT @@ -406,10 +405,6 @@ H5G_node_cmp2(void *_lt_key, void *_udata, void *_rt_key) HDassert(lt_key); HDassert(rt_key); - /* Get base address of heap */ - base = (const char *)H5HL_offset_into(udata->heap, (size_t)0); - HDassert(base); - /* Get pointers to string names */ if((s1 = (const char *)H5HL_offset_into(udata->heap, lt_key->offset)) == NULL) HGOTO_ERROR(H5E_SYM, H5E_CANTGET, FAIL, "unable to get key name") @@ -458,7 +453,6 @@ H5G_node_cmp3(void *_lt_key, void *_udata, void *_rt_key) H5G_node_key_t *lt_key = (H5G_node_key_t *) _lt_key; H5G_node_key_t *rt_key = (H5G_node_key_t *) _rt_key; const char *s; - const char *base; /* Base of heap */ herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_NOAPI_NOINIT @@ -468,10 +462,6 @@ H5G_node_cmp3(void *_lt_key, void *_udata, void *_rt_key) HDassert(lt_key); HDassert(rt_key); - /* Get base address of heap */ - base = (const char *)H5HL_offset_into(udata->heap, (size_t)0); - HDassert(base); - /* left side */ if((s = (const char *)H5HL_offset_into(udata->heap, lt_key->offset)) == NULL) HGOTO_ERROR(H5E_SYM, H5E_CANTGET, FAIL, "unable to get key name") @@ -525,7 +515,6 @@ H5G_node_found(H5F_t *f, hid_t dxpl_id, haddr_t addr, const void H5_ATTR_UNUSED unsigned lt = 0, idx = 0, rt; int cmp = 1; const char *s; - const char *base; /* Base of heap */ htri_t ret_value = TRUE; /* Return value */ FUNC_ENTER_NOAPI_NOINIT @@ -543,10 +532,6 @@ H5G_node_found(H5F_t *f, hid_t dxpl_id, haddr_t addr, const void H5_ATTR_UNUSED if(NULL == (sn = (H5G_node_t *)H5AC_protect(f, dxpl_id, H5AC_SNODE, addr, f, H5AC__READ_ONLY_FLAG))) HGOTO_ERROR(H5E_SYM, H5E_CANTLOAD, FAIL, "unable to protect symbol table node") - /* Get base address of heap */ - base = (const char *)H5HL_offset_into(udata->common.heap, (size_t)0); - HDassert(base); - /* * Binary search. */ @@ -624,7 +609,6 @@ H5G_node_insert(H5F_t *f, hid_t dxpl_id, haddr_t addr, H5G_node_t *sn = NULL, *snrt = NULL; unsigned sn_flags = H5AC__NO_FLAGS_SET, snrt_flags = H5AC__NO_FLAGS_SET; const char *s; - const char *base; /* Base of heap */ unsigned lt = 0, rt; /* Binary search cntrs */ int cmp = 1, idx = -1; H5G_node_t *insert_into = NULL; /*node that gets new entry*/ @@ -649,10 +633,6 @@ H5G_node_insert(H5F_t *f, hid_t dxpl_id, haddr_t addr, if(NULL == (sn = (H5G_node_t *)H5AC_protect(f, dxpl_id, H5AC_SNODE, addr, f, H5AC__NO_FLAGS_SET))) HGOTO_ERROR(H5E_SYM, H5E_CANTLOAD, H5B_INS_ERROR, "unable to protect symbol table node") - /* Get base address of heap */ - base = (const char *)H5HL_offset_into(udata->common.heap, (size_t)0); - HDassert(base); - /* * Where does the new symbol get inserted? We use a binary search. */ @@ -815,10 +795,6 @@ H5G_node_remove(H5F_t *f, hid_t dxpl_id, haddr_t addr, void *_lt_key/*in,out*/, if(udata->common.name != NULL) { H5O_link_t lnk; /* Constructed link for replacement */ size_t link_name_len; /* Length of string in local heap */ - const char *base; /* Base of heap */ - - /* Get base address of heap */ - base = (const char *)H5HL_offset_into(udata->common.heap, (size_t)0); /* Find the name with a binary search */ rt = sn->nsyms; @@ -1019,7 +995,6 @@ H5G__node_iterate(H5F_t *f, hid_t dxpl_id, const void H5_ATTR_UNUSED *_lt_key, h /* Get the pointer to the name of the link in the heap */ if((name = (const char *)H5HL_offset_into(udata->heap, ents[u].name_off)) == NULL) HGOTO_ERROR(H5E_SYM, H5E_CANTGET, H5_ITER_ERROR, "unable to get symbol table node name") - HDassert(name); /* Convert the entry to a link */ if(H5G__ent_to_link(&lnk, udata->heap, &ents[u], name) < 0) @@ -1362,7 +1337,6 @@ H5G__node_copy(H5F_t *f, hid_t dxpl_id, const void H5_ATTR_UNUSED *_lt_key, hadd /* Determine name of source object */ if((name = (const char *)H5HL_offset_into(heap, src_ent->name_off)) == NULL) HGOTO_ERROR(H5E_OHDR, H5E_CANTGET, H5_ITER_ERROR, "unable to get source object name") - HDassert(name); /* Set copied metadata tag */ H5_BEGIN_TAG(dxpl_id, H5AC__COPIED_TAG, H5_ITER_ERROR); @@ -1447,7 +1421,6 @@ H5G__node_build_table(H5F_t *f, hid_t dxpl_id, const void H5_ATTR_UNUSED *_lt_ke /* Get pointer to link's name in the heap */ if((name = (const char *)H5HL_offset_into(udata->heap, sn->entry[u].name_off)) == NULL) HGOTO_ERROR(H5E_SYM, H5E_CANTGET, H5_ITER_ERROR, "unable to get symbol table link name") - HDassert(name); /* Determine the link to operate on in the table */ linkno = udata->ltable->nlinks++; @@ -1549,29 +1522,29 @@ H5G_node_debug(H5F_t *f, hid_t dxpl_id, haddr_t addr, FILE * stream, int indent, HGOTO_ERROR(H5E_SYM, H5E_CANTLOAD, FAIL, "unable to debug B-tree node"); } /* end if */ else { - fprintf(stream, "%*sSymbol Table Node...\n", indent, ""); - fprintf(stream, "%*s%-*s %s\n", indent, "", fwidth, + HDfprintf(stream, "%*sSymbol Table Node...\n", indent, ""); + HDfprintf(stream, "%*s%-*s %s\n", indent, "", fwidth, "Dirty:", sn->cache_info.is_dirty ? "Yes" : "No"); - fprintf(stream, "%*s%-*s %u\n", indent, "", fwidth, + HDfprintf(stream, "%*s%-*s %u\n", indent, "", fwidth, "Size of Node (in bytes):", (unsigned)sn->node_size); - fprintf(stream, "%*s%-*s %u of %u\n", indent, "", fwidth, + HDfprintf(stream, "%*s%-*s %u of %u\n", indent, "", fwidth, "Number of Symbols:", sn->nsyms, (unsigned)(2 * H5F_SYM_LEAF_K(f))); indent += 3; fwidth = MAX(0, fwidth - 3); for(u = 0; u < sn->nsyms; u++) { - fprintf(stream, "%*sSymbol %u:\n", indent - 3, "", u); + HDfprintf(stream, "%*sSymbol %u:\n", indent - 3, "", u); if(heap) { const char *s = (const char *)H5HL_offset_into(heap, sn->entry[u].name_off); if(s) - fprintf(stream, "%*s%-*s `%s'\n", indent, "", fwidth, "Name:", s); + HDfprintf(stream, "%*s%-*s `%s'\n", indent, "", fwidth, "Name:", s); } /* end if */ else - fprintf(stream, "%*s%-*s\n", indent, "", fwidth, "Warning: Invalid heap address given, name not displayed!"); + HDfprintf(stream, "%*s%-*s\n", indent, "", fwidth, "Warning: Invalid heap address given, name not displayed!"); H5G__ent_debug(sn->entry + u, stream, indent, fwidth, heap); } /* end for */ diff --git a/src/H5Gstab.c b/src/H5Gstab.c index 7750671..4dc06ca 100644 --- a/src/H5Gstab.c +++ b/src/H5Gstab.c @@ -724,9 +724,8 @@ H5G_stab_get_name_by_idx_cb(const H5G_entry_t *ent, void *_udata) if((name = (const char *)H5HL_offset_into(udata->heap, name_off)) == NULL) HGOTO_ERROR(H5E_SYM, H5E_CANTGET, FAIL, "unable to get symbol table link name") - HDassert(name); - udata->name = H5MM_strdup(name); - HDassert(udata->name); + if((udata->name = H5MM_strdup(name)) == NULL) + HGOTO_ERROR(H5E_SYM, H5E_CANTGET, FAIL, "unable to duplicate symbol table link name") done: FUNC_LEAVE_NOAPI(ret_value) @@ -948,7 +947,6 @@ H5G_stab_lookup_by_idx_cb(const H5G_entry_t *ent, void *_udata) /* Get a pointer to the link name */ if((name = (const char *)H5HL_offset_into(udata->heap, ent->name_off)) == NULL) HGOTO_ERROR(H5E_SYM, H5E_CANTGET, FAIL, "unable to get symbol table link name") - HDassert(name); /* Convert the entry to a link */ if(H5G__ent_to_link(udata->lnk, udata->heap, ent, name) < 0) diff --git a/src/H5Oefl.c b/src/H5Oefl.c index 2273289..7d78caf 100644 --- a/src/H5Oefl.c +++ b/src/H5Oefl.c @@ -151,7 +151,8 @@ H5O_efl_decode(H5F_t *f, hid_t dxpl_id, H5O_t H5_ATTR_UNUSED *open_oh, if((s = (const char *)H5HL_offset_into(heap, mesg->slot[u].name_offset)) == NULL) HGOTO_ERROR(H5E_SYM, H5E_CANTGET, NULL, "unable to get external file name") - HDassert(s && *s); + if(*s == NULL) + HGOTO_ERROR(H5E_SYM, H5E_CANTGET, NULL, "invalid external file name") mesg->slot[u].name = H5MM_xstrdup (s); HDassert(mesg->slot[u].name); diff --git a/test/bad_offset.h5 b/test/bad_offset.h5 index 6500ffe..231dca2 100644 Binary files a/test/bad_offset.h5 and b/test/bad_offset.h5 differ diff --git a/test/gen_bad_offset.c b/test/gen_bad_offset.c index 2be4af1..82e94cd 100644 --- a/test/gen_bad_offset.c +++ b/test/gen_bad_offset.c @@ -27,9 +27,9 @@ /*------------------------------------------------------------------------- * Function: main - * Generate an HDF5 file with groups, datasets and symbolic links. * - * After this file is generated, write bad offset values to + * Generate an HDF5 file with groups, datasets and symbolic links. + * After the file is generated, write bad offset values to * the heap at 3 locations in the file: * (A) Open the file: * fd = HDopen(TESTFILE, O_RDWR, 0663); @@ -40,7 +40,7 @@ * "/dsetA": replace name offset into private heap "72" by bad offset * (3) HDlseek(fd, (HDoff_t)1616, SEEK_SET); * /soft_one: replace link value offset in the scratch pad "32" by bad offset - * (C) Write the bad offset value to the file: + * (C) Write the bad offset value to the file for (1), (2) and (3): * write(fd, &val, sizeof(val)); * * Note: if the groups/datasets/symbolic links are changed in the file, @@ -55,6 +55,8 @@ main(void) { hid_t fid = -1, gid1 = -1, gid2 = -1; /* File and group IDs */ hid_t did = -1, sid = -1; /* Dataset and dataspace IDs */ + int fd = -1; /* File descriptor */ + int64_t val = 999; /* Bad offset value */ /* Create the test file */ if((fid = H5Fcreate(TESTFILE, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT)) < 0) @@ -96,6 +98,39 @@ main(void) if(H5Fclose(fid) < 0) FAIL_STACK_ERROR + /* + * Write bad offset values at 3 locations in the file + */ + + /* Open the file */ + if((fd = HDopen(TESTFILE, O_RDWR, 0663)) < 0) + FAIL_STACK_ERROR + + /* Position the file for /group1/group2: replace heap offset "8" by bad offset */ + if(HDlseek(fd, (HDoff_t)880, SEEK_SET) < 0) + FAIL_STACK_ERROR + /* Write the bad offset value to the file */ + if(HDwrite(fd, &val, sizeof(val)) < 0) + FAIL_STACK_ERROR + + /* Position the file for /dsetA: replace name offset into private heap "72" by bad offset */ + if(HDlseek(fd, (HDoff_t)1512, SEEK_SET) < 0) + FAIL_STACK_ERROR + /* Write the bad offset value to the file */ + if(HDwrite(fd, &val, sizeof(val)) < 0) + FAIL_STACK_ERROR + + /* Position the file for /soft_one: replace link value offset in the scratch pad "32" by bad offset */ + if(HDlseek(fd, (HDoff_t)1616, SEEK_SET) < 0) + FAIL_STACK_ERROR + /* Write the bad offset value to the file */ + if(HDwrite(fd, &val, sizeof(val)) < 0) + FAIL_STACK_ERROR + + /* Close the file */ + if(HDclose(fd) < 0) + FAIL_STACK_ERROR + return EXIT_SUCCESS; error: diff --git a/test/tmisc.c b/test/tmisc.c index 242e55a..102325a 100644 --- a/test/tmisc.c +++ b/test/tmisc.c @@ -5574,7 +5574,7 @@ test_misc(void) test_misc30(); /* Exercise local heap loading bug where free lists were getting dropped */ test_misc31(); /* Test Reentering library through deprecated routines after H5close() */ test_misc32(); /* Test filter memory allocation functions */ - test_misc33(); /* ??? */ + test_misc33(); /* Test to verify that H5HL_offset_into() returns error if offset exceeds heap block */ } /* test_misc() */ -- cgit v0.12