Fix several warnings #720

jhendersonHDF · 2021-06-03T20:46:47Z

Fixes a few issues uncovered from GCC's static analysis, along with several pesky warnings such as strncpy with a size equal to the source object's size, all of the unused H5FD package variable warnings, etc.

java/src/jni/h5util.c

src/H5.c

jhendersonHDF · 2021-06-03T20:50:07Z

src/H5CX.c


    FUNC_ENTER_NOAPI_NOINIT_NOERR

    /* Sanity check */
    head = H5CX_get_my_context(); /* Get the pointer to the head of the API context, for this thread */
    HDassert(head && *head);

-    FUNC_LEAVE_NOAPI((*head)->ctx.coll_metadata_read)


In this and the following changes, (*head) could be accessed while head is NULL if the FUNC_ENTER fails. Instead, set the return value only when FUNC_ENTER succeeds; return a default value otherwise.

No, there's no way for this FUNC_ENTER macro to fail, the compiler is just confused.

No objection to the change if it quiets the warning though. :-)

Ah, well even if the macro can't fail, this will also be problematic if the pkg_init var is not TRUE, because head only gets set if the package has been initialized. GCC's not confused ;)

jhendersonHDF · 2021-06-03T20:50:46Z

src/H5Dchunk.c

@@ -7483,7 +7483,7 @@ H5D__chunk_iter_cb(const H5D_chunk_rec_t *chunk_rec, void *udata)
    /* Check for callback failure and pass along return value */
    if ((ret_value = (data->op)(chunk_rec->scaled, chunk_rec->filter_mask, chunk_rec->chunk_addr,
                                chunk_rec->nbytes, data->op_data)) < 0)
-        HERROR(H5E_DATASET, H5E_CANTNEXT, "iteration operator failed");
+        HGOTO_ERROR(H5E_DATASET, H5E_CANTNEXT, ret_value, "iteration operator failed")


eliminates two pesky warnings about the unused 'done:' label, along with an unused 'err_occurred' variable from the FUNC_ENTER macro.

This isn't the correct idiom for the library. To fix this problem, the routine should be using FUNC_ENTER_STATIC_NOERR

This seems a little bit misled? Why use a NOERR macro when we're going to push an error onto the stack?

It still doesn't make much sense to me to use a NOERR FUNC_ENTER and then issue an error, but changed this back to HERROR with a FUNC_ENTER_STATIC_NOERR and removed the done label.

jhendersonHDF · 2021-06-03T20:51:33Z

src/H5E.c

@@ -639,7 +639,7 @@ H5E__get_class_name(const H5E_cls_t *cls, char *name, size_t size)

    /* Set the user's buffer, if provided */
    if (name) {
-        HDstrncpy(name, cls->cls_name, MIN((size_t)(len + 1), size));


While using MIN here can make strncpy do less work in the case that len + 1 is smaller than size, GCC isn't intelligent enough to figure out what we're doing here, so stick to something sensible.

jhendersonHDF · 2021-06-03T20:52:30Z

src/H5Eint.c

@@ -129,7 +129,7 @@ H5E__get_msg(const H5E_msg_t *msg, H5E_type_t *type, char *msg_str, size_t size)

    /* Copy the message into the user's buffer, if given */
    if (msg_str) {
-        HDstrncpy(msg_str, msg->msg, MIN((size_t)(len + 1), size));


Same as above

jhendersonHDF · 2021-06-03T20:52:51Z

src/H5FDhdfs.c

 /* This source code file is part of the H5FD driver module */
 #include "H5FDdrvr_module.h"
+#endif


Eliminates a giant warning about unused H5FD package variables.

jhendersonHDF · 2021-06-03T20:52:57Z

src/H5FDmirror.c

 #include "H5private.h" /* Generic Functions        */

 #ifdef H5_HAVE_MIRROR_VFD

+#include "H5FDdrvr_module.h" /* This source code file is part of the H5FD driver module */
+


Same as above.

jhendersonHDF · 2021-06-03T20:53:56Z

src/H5FDmulti.c

@@ -624,7 +624,7 @@ H5FD_multi_sb_encode(H5FD_t *_file, char *name /*out*/, unsigned char *buf /*out
    H5Eclear2(H5E_DEFAULT);

    /* Name and version number */
-    strncpy(name, "NCSAmulti", (size_t)8);
+    strncpy(name, "NCSAmult", (size_t)9);


Previously was trying to copy 8 bytes out of a 10 byte (including NUL terminator) string?

Nope - there's only room for 8 bytes in the "Driver ID" field for the file format: https://portal.hdfgroup.org/display/HDF5/File+Format+Specification

I understand that. However,

"NCSAmulti" is 10 bytes long. Let's not confuse the issue by copying from a string that's larger than what we're actually using. Just format the string according to what we're actually going to use, e.g. "NCSAmult".

The header comment specifically says a 9-byte buffer is used: 8 bytes for the file format piece + a NUL terminator. Therefore, I want to copy at most 9 bytes from the source. I realize that we immediately manually NUL terminate the string, but I prefer to use strncpy how it's intended.

jhendersonHDF · 2021-06-03T20:55:55Z

src/H5FDmulti.c

@@ -660,7 +660,7 @@ H5FD_multi_sb_encode(H5FD_t *_file, char *name /*out*/, unsigned char *buf /*out
    p = buf + 8 + nseen * 2 * 8;
    UNIQUE_MEMBERS (file->fa.memb_map, mt) {
        size_t n = strlen(file->fa.memb_name[mt]) + 1;
-        strncpy((char *)p, file->fa.memb_name[mt], n);
+        strcpy((char *)p, file->fa.memb_name[mt]);


Using strcpy here is no less safe than strncpy. We don't know the total size of the output buffer (the API doesn't give us that info), and using strncpy with a size based on the source length will throw a warning anyway.

Regardless which one, shouldn't it be HD...?

Ah, sure, that's a good point. Let me fix all the usages that were never converted over.

It turns out that the multi driver uses only public HDF5 headers, so it doesn't use the HD-prefixed versions of standard library/system calls.

The stdio VFD is the same way. They were intended as "demo" VFDs to show people how to write them using the public API (multi = multi-file VFD example, stdio = single-file VFD example).

jhendersonHDF · 2021-06-03T20:56:46Z

src/H5FDros3.c

 /* This source code file is part of the H5FD driver module */
 #include "H5FDdrvr_module.h"
+#endif


Another eliminated H5FD package variable warning.

jhendersonHDF · 2021-06-03T20:57:22Z

src/H5FDsplitter.c

-    info->rw_fapl_id = H5P_FILE_ACCESS_DEFAULT; /* pre-set value */
-    info->wo_fapl_id = H5P_FILE_ACCESS_DEFAULT; /* pre-set value */
+    HDstrncpy(info->wo_path, vfd_config->wo_path, H5FD_SPLITTER_PATH_MAX + 1);
+    info->wo_path[H5FD_SPLITTER_PATH_MAX] = '\0';


Make sure to copy the whole path and ensure it's NUL-terminated, otherwise GCC complains about truncated output.

jhendersonHDF · 2021-06-03T20:57:40Z

src/H5Fsuper.c

@@ -1043,7 +1043,7 @@ H5F__super_read(H5F_t *f, H5P_genplist_t *fa_plist, hbool_t initial_read)
                HDONE_ERROR(H5E_FILE, H5E_CANTUNPIN, FAIL, "unable to unpin driver info")

            /* Evict the driver info block from the cache */
-            if (H5AC_expunge_entry(f, H5AC_DRVRINFO, sblock->driver_addr, H5AC__NO_FLAGS_SET) < 0)
+            if (sblock && H5AC_expunge_entry(f, H5AC_DRVRINFO, sblock->driver_addr, H5AC__NO_FLAGS_SET) < 0)


GCC complains about a potential dereference of a NULL sblock here.

This is trickier than that - the sblock should be set to NULL after the H5AC_unprotect, about 15 lines earlier. But, if that's the case, how's the expunge supposed to work? There's a bigger algorithmic problem here.

Sure, but the algorithm is beyond me and adding an sblock check here theoretically won't do more harm than good. If sblock actually were NULL here, we'd immediately crash rather than H5AC_expunge_entry doing something "bad". Now, we just skip the expunge anyway.

As far as I can tell, the H5AC_unprotect call flushes the entry, but I don't see anywhere that the entry is freed/set to NULL, especially since H5AC_unprotect only accepts a single level of pointer indirection. Since the code following the expunge also checks to make sure sblock is valid before using it, this seems like a reasonable change, but maybe @jrmainzer would have a better idea?

jhendersonHDF · 2021-06-03T20:59:04Z

src/H5PL.c

@@ -359,7 +359,7 @@ H5PLget(unsigned int idx, char *path_buf, size_t buf_size)

    /* If the path buffer is not NULL, copy the path to the buffer */
    if (path_buf) {
-        HDstrncpy(path_buf, path, MIN((size_t)(path_len + 1), buf_size));


Same comment about GCC being confused about the usage of MIN here.

jhendersonHDF · 2021-06-03T20:59:10Z

src/H5Pdapl.c

@@ -1453,7 +1453,7 @@ H5Pget_efile_prefix(hid_t plist_id, char *prefix /*out*/, size_t size)
        /* Copy to user's buffer, if given */
        len = HDstrlen(my_prefix);
        if (prefix) {
-            HDstrncpy(prefix, my_prefix, MIN(len + 1, size));


Same as above

jhendersonHDF · 2021-06-03T20:59:14Z

src/H5Pdapl.c

@@ -1543,7 +1543,7 @@ H5Pget_virtual_prefix(hid_t plist_id, char *prefix /*out*/, size_t size)
        /* Copy to user's buffer, if given */
        len = HDstrlen(my_prefix);
        if (prefix) {
-            HDstrncpy(prefix, my_prefix, MIN(len + 1, size));


Same as above

jhendersonHDF · 2021-06-03T21:00:06Z

src/H5Pencdec.c

@@ -333,7 +333,7 @@ H5P__encode_cb(H5P_genprop_t *prop, void *_udata)
        /* Encode (or not, if the 'encode' flag is off) the property's name */
        prop_name_len = HDstrlen(prop->name) + 1;
        if (udata->encode) {
-            HDstrncpy((char *)*(udata->pp), prop->name, prop_name_len);


Again, we don't know the size of the output buffer, so using strcpy here is no less safe than strncpy and strncpy gives a warning since we're just giving it the length of the source being copied.

jhendersonHDF · 2021-06-03T21:03:19Z

testpar/t_span_tree.c

@@ -35,6 +35,8 @@
 #include "H5private.h"
 #include "testphdf5.h"

+#define LOWER_DIM_SIZE_COMP_TEST__RUN_TEST__DEBUG 0


Moving this up in the file eliminates a warning about it being undefined and evaluating to 0

jhendersonHDF · 2021-06-03T21:03:49Z

tools/src/h5dump/h5dump_ddl.c

@@ -1421,10 +1429,10 @@ lnk_search(const char *path, const H5L_info2_t *li, void *_op_data)
    else {
        if (k == 2) {
            HDstrcpy(search_name, "/");
-            HDstrncat(search_name, op_name, search_len + 1);
+            HDstrcat(search_name, op_name);


Already know the buffer size, use strcat to not generate a warning.

derobins · 2021-06-04T17:18:12Z

test/objcopy.c

+    (void)fcpl_src;
+    (void)fcpl_dst;
+    (void)src_fapl;
+    (void)dst_fapl; /* Silence compiler */


One of these days I'm going to go through the library and replace all the H5_ATTR_UNUSED macro nonsense with void statements, which are more portable and less of a maintenance headache.

I'm all for this, though we should probably set up a variadic macro to do any number of arguments in one go so we don't have this big stack of them.

* Use internal version of H5Eprint2 to avoid possible stack overflow (#661) * Add support for parallel filters to h5repack (#832) * Allow parallel filters feature for comm size of 1 (#840) * Avoid popping API context when one wasn't pushed (#848) * Fix several warnings (#720) * Don't allow H5Pset(get)_all_coll_metadata_ops for DXPLs (#1201) * Fix free list tracking and cleanup cast alignment warnings (#1288) * Fix free list tracking and cleanup cast alignment warnings * Add free list tracking code to H5FL 'arr' routines * Fix usage of several HDfprintf format specifiers after HDfprintf removal (#1324) * Use appropriate printf format specifiers for haddr_t and hsize_t types directly (#1340) * Fix H5ACmpio dirty bytes creation debugging (#1357) * Fix documentation for H5D_space_status_t enum values (#1372) * Parallel rank0 deadlock fixes (#1183) * Fix several places where rank 0 can skip past collective MPI operations on failure * Committing clang-format changes Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * Fix a few issues noted by LGTM (#1421) * Fix cache sanity checking code by moving functions to wider scope (#1435) * Fix metadata cache bug when resizing a pinned/protected entry (v2) (#1463) * Disable memory alloc sanity checks by default for Autotools debug builds (#1468) * Committing clang-format changes Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

jhendersonHDF requested review from bmribler, byrnHDF, derobins, fortnern, jrmainzer, qkoziol, rawarren, soumagne and vchoi-hdfgroup as code owners June 3, 2021 20:46