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

7 files changed, 65 insertions, 51 deletions

diff --git a/src/H5Glink.c b/src/H5Glink.c
index fe49458..255add0 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 7de9ec9..2c4b840 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)))
         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_WRITE)))
         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);
 
         /* Insert the new object in the destination file's group */
         /* (Don't increment the link count - that's already done above for hard links) */
@@ -1440,7 +1414,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++;
@@ -1542,29 +1515,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 01dbdbc..0eb7b0b 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 c4b1d96..26ac63f 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
--- a/test/bad_offset.h5
+++ b/test/bad_offset.h5
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 980c9ea..aae1a99 100644
--- a/test/tmisc.c
+++ b/test/tmisc.c
@@ -5525,7 +5525,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() */