Various fixes #1640

mathstuf · 2022-04-14T14:05:51Z

Various fixes from VTK's bundled HDF5 that we've applied that seems relevant for upstream.

mathstuf · 2022-04-14T14:43:08Z

I can split this into multiple PRs if that is preferred as well.

byrnHDF · 2022-04-14T14:57:11Z

config/cmake/H5pubconf.h.in

@@ -190,9 +190,6 @@
 /* Define to 1 if you have the `curl' library (-lcurl). */
 #cmakedefine H5_HAVE_LIBCURL @H5_HAVE_LIBCURL@

-/* Define to 1 if you have the `dl' library (-ldl). */


We still maintain two build systems: CMake and autotools/autoconf.
This file needs to remain in sync with the file generated by autoconf until such time that we no longer support autoconf.

Nothing cares if this define is present or not, so it isn't doing anything in the header.

We keep this file in sync with the one produced by Autoconf so it's easier to eyeball them to make sure we aren't missing something in the other build system.

A lot of these header files are useless, though, and have already been removed from configure.ac.

byrnHDF · 2022-04-14T14:59:25Z

config/cmake_ext_mod/ConfigureChecks.cmake

 if (MINGW OR NOT WINDOWS)
  CHECK_LIBRARY_EXISTS_CONCAT ("m" ceil     ${HDF_PREFIX}_HAVE_LIBM)
-  CHECK_LIBRARY_EXISTS_CONCAT ("dl" dlopen     ${HDF_PREFIX}_HAVE_LIBDL)


Does mingw not need these? Mingw generates windows objects on linux, right?

Yes, but there is no libdl in MinGW as it uses LoadLibrary. Maybe you're thinking of Cygwin?

hl/src/H5LTanalyze.c

jhendersonHDF · 2022-04-14T15:02:54Z

src/H5system.c

-                HDstrncat(full_path, H5_DIR_SEPS, path_len - (cwdlen + 1));
-            HDstrncat(full_path, new_name, path_len - (cwdlen + 1) - HDstrlen(H5_DIR_SEPS));
+                HDstrcat(full_path, H5_DIR_SEPS);
+            HDstrcat(full_path, new_name);


This change seems like it will possibly be reintroducing a vulnerability by not calculating how much space is left in full_path

The correct length is allocated right above this.

config/cmake/HDFCompilerFlags.cmake

byrnHDF · 2022-04-14T15:09:49Z

src/CMakeLists.txt

@@ -1043,76 +1043,40 @@ target_compile_options(H5detect
 )
 set (lib_prog_deps ${lib_prog_deps} H5detect)

-# check if a pregenerated H5Tinit.c file is present
-if (NOT EXISTS "${HDF5_GENERATED_SOURCE_DIR}/H5Tinit.c")


This check is if we are cross-compiling with a pre-generated file - because the host system cannot generate a file for the target system. How is it handled in your changes?

Hrm. This causes problems because rerunning the configure after the build happens causes it to detect the "pre-generated" file and confuses reconfigures. I think another approach for cross-compilations where the source directory gets the to-be-used file is used instead of overloading the existence of the output like this.

If there is a cross-compiling emulator available (e.g., Wine when doing MinGW on Linux), it will be able to run the binary. So it's not just "cross compiling, therefore impossible".

byrnHDF · 2022-04-14T15:10:33Z

src/CMakeLists.txt

+      ${HDF5_SOURCE_DIR}/bin/batch/${HDF5_BATCH_H5DETECT_SCRIPT}.in.cmake
+      ${HDF5_BINARY_DIR}/${HDF5_BATCH_H5DETECT_SCRIPT} ESCAPE_QUOTES @ONLY
+  )
+  add_custom_command (


Does this form of add_custom_command work correctly with the ninja system?

Yes, it's what I tested it with. I'm not really sure why the stamp gymnastics were done previously as they just overcomplicated it instead of making anything actually work (AFAICT).

gnuoyd · 2022-04-14T15:11:38Z

hl/src/H5LTanalyze.c

@@ -310,6 +310,7 @@
 #include <string.h>
 #include <errno.h>
 #include <stdlib.h>
+#include <stdint.h>


Changes should be made in H5LTanalyze.l. H5LTanalyze.c is generated by lex from H5LTanalyze.l.

The commit message mentions that flex generates the bits that it doesn't include for. I can include it there, but flex should really be adding the include for its generated code. I'll add a comment there to that effect.

This is fixed in develop

derobins

The proposed changes need justification/discussion

gnuoyd · 2022-04-14T15:51:45Z

src/H5system.c

-                HDstrncat(full_path, H5_DIR_SEPS, path_len - (cwdlen + 1));
-            HDstrncat(full_path, new_name, path_len - (cwdlen + 1) - HDstrlen(H5_DIR_SEPS));
+                HDstrcat(full_path, H5_DIR_SEPS);
+            HDstrcat(full_path, new_name);


How about a defensive rewrite using snprintf?

int rc = snprintf(full_path, path_len, "%s%s%s", cwdpath, H5_CHECK_DELIMITER(cwdpath[cwdlen - 1]) ? "" : H5_DIR_SEPS, new_name); if (rc < 0 || (size_t)rc >= path_len) { HGOTO_ERROR(H5E_INTERNAL, H5E_CANTCOPY, FAIL, "buffer would overflow") }

I think that would certainly be a bit less prone to error, though maybe a little bit less clean. I'd be for doing that though.

I'm all for using safer C string manip functions :) .

However, if there's an overflow, that is a logic error in this function as path_len is calcuated right above.

These calls did `strop(dst, src, strlen(src))` in some form or another. The size to these operations is the size of the *destination* buffer; the source size is implicit with `NUL` termination.

Other locations check if it is defined and in cases where it is not available, it is undefined, not 0.

These were checked for at configure time, but nothing in the code cared about their results. Remove the checks.

This is actually a bug in flex, not the grammar file.

mathstuf · 2022-04-14T17:13:25Z

The proposed changes need justification/discussion

Please see the commit messages. Are there any that are deficient?

In release builds, `assert`-like macros disappear and cause some variables to be detected as unused. Add a "fake" usage to let the compiler know that things are as intended.

mathstuf · 2022-07-22T19:29:26Z

Oh, there's stuff to perserve here.

derobins · 2022-07-22T19:30:43Z

Yeah, I've been meaning to go over this again to see what's up with all the changes.

derobins · 2022-07-26T17:42:51Z

src/H5Tref.c

@@ -656,6 +656,7 @@ H5T__ref_mem_write(H5VL_object_t *src_file, const void *src_buf, size_t src_size
    HDassert(dst_buf);
    HDassert(dst_size == H5T_REF_MEM_SIZE);
    HDcompile_assert(sizeof(*dst_ref) == sizeof(tmp_ref));
+    (void)dst_size;


This is fixed in develop

derobins · 2022-07-26T17:46:48Z

src/H5Oshared.c

@@ -590,6 +590,8 @@ H5O__shared_copy_file(H5F_t H5_ATTR_NDEBUG_UNUSED *file_src, H5F_t *file_dst,
    HDassert(shared_dst);
    HDassert(recompute_size);
    HDassert(cpy_info);
+    (void)file_src;
+    (void)cpy_info;


Fixed in develop

derobins · 2022-07-26T17:48:07Z

src/H5Oint.c

@@ -3045,6 +3045,8 @@ H5O__free(H5O_t *oh, hbool_t force)
                oh->ndecode_dirtied--;
            else if (!force)
                HDassert(oh->mesg[u].dirty == 0);
+#else
+            (void)force;


Fixed in develop

derobins · 2022-07-26T17:51:27Z

src/H5Ocache.c

@@ -750,6 +752,7 @@ H5O__cache_chk_deserialize(const void *image, size_t len, void *_udata, hbool_t
    HDassert(udata);
    HDassert(udata->oh);
    HDassert(dirty);
+    (void)len;


len is used below

derobins · 2022-07-26T17:51:45Z

src/H5Ocache.c

@@ -302,6 +303,7 @@ H5O__cache_deserialize(const void *image, size_t len, void *_udata, hbool_t *dir
    HDassert(udata->common.f);
    HDassert(udata->common.cont_msg_info);
    HDassert(dirty);
+    (void)len;


len is used below

derobins · 2022-07-26T17:51:58Z

src/H5Ocache.c

@@ -197,6 +197,7 @@ H5O__cache_get_final_load_size(const void *image, size_t H5_ATTR_NDEBUG_UNUSED i
    HDassert(udata);
    HDassert(actual_len);
    HDassert(*actual_len == image_len);
+    (void)image_len;


Fixed in develop

derobins · 2022-07-26T17:52:36Z

src/H5Oalloc.c

-    }  /* end if */
+    } /* end if */
+#else
+    (void)f;


Fixed in develop

derobins · 2022-07-26T17:55:02Z

src/H5FDsec2.c

@@ -1085,6 +1085,7 @@ H5FD__sec2_ctl(H5FD_t *_file, uint64_t op_code, uint64_t flags, const void H5_AT

    /* Sanity checks */
    HDassert(file);
+    (void)file;


Fixed in develop

derobins · 2022-07-26T17:56:27Z

src/H5EAtest.c

-    uint64_t *     elmt = (uint64_t *)_elmt;          /* Convenience pointer to native elements */
-    const uint8_t *raw  = (const uint8_t *)_raw;      /* Convenience pointer to raw elements */
+#else
+    (void)_ctx;


Fixed in develop

derobins · 2022-07-26T17:57:51Z

src/H5Dcontig.c

@@ -614,6 +614,7 @@ H5D__contig_may_use_select_io(const H5D_io_info_t *io_info, H5D_io_op_type_t op_
    HDassert(io_info);
    HDassert(dataset);
    HDassert(op_type == H5D_IO_OP_READ || op_type == H5D_IO_OP_WRITE);
+    (void)dataset;


Fixed in develop

derobins · 2022-07-26T17:58:52Z

src/H5Dchunk.c

@@ -8114,6 +8114,7 @@ H5D__chunk_iter(const H5D_t *dset, H5D_chunk_iter_op_t op, void *op_data)
    HDassert(layout);
    HDassert(rdcc);
    HDassert(H5D_CHUNKED == layout->type);
+    (void)layout;


Fixed in develop

derobins · 2022-07-26T18:01:24Z

src/H5Aint.c

@@ -2148,6 +2148,7 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, hbool_t *recompute_s
    HDassert(file_dst);
    HDassert(cpy_info);
    HDassert(!cpy_info->copy_without_attr);
+    (void)cpy_info;


Fixed in develop

src/H5detect.c

derobins · 2023-06-07T21:09:40Z

I think we've pulled everything we can out of this. The one thing that is left is the cross-compiling bits, but I'm going to leave that to a future discussion.

mathstuf requested review from jhendersonHDF, fortnern, soumagne, vchoi-hdfgroup, ChristopherHogan, gnuoyd, derobins, jrmainzer, qkoziol, bmribler, byrnHDF and lrknox as code owners April 14, 2022 14:05

mathstuf force-pushed the various-fixes branch from 901fea1 to 0ab76f8 Compare April 14, 2022 14:11

byrnHDF reviewed Apr 14, 2022

View reviewed changes

hl/src/H5LTanalyze.c Show resolved Hide resolved

jhendersonHDF reviewed Apr 14, 2022

View reviewed changes

byrnHDF reviewed Apr 14, 2022

View reviewed changes

config/cmake/HDFCompilerFlags.cmake Outdated Show resolved Hide resolved

byrnHDF reviewed Apr 14, 2022

View reviewed changes

gnuoyd reviewed Apr 14, 2022

View reviewed changes

derobins requested changes Apr 14, 2022

View reviewed changes

gnuoyd reviewed Apr 14, 2022

View reviewed changes

mathstuf added 4 commits April 14, 2022 13:11

strncat, strncpy: replace with strcat and strcpy

a3c8caa

These calls did `strop(dst, src, strlen(src))` in some form or another. The size to these operations is the size of the *destination* buffer; the source size is implicit with `NUL` termination.

H5Tconv: fix H5T_CONV_INTERNAL_ checks

3b9dd4f

Other locations check if it is defined and in cases where it is not available, it is undefined, not 0.

cmake: remove unused configure checks

df75827

These were checked for at configure time, but nothing in the code cared about their results. Remove the checks.

H5LTanalyze: include stdint.h before INT8_MAX are checked

f868b3e

This is actually a bug in flex, not the grammar file.

warnings: mark conditionally used parameters as used

360b173

In release builds, `assert`-like macros disappear and cause some variables to be detected as unused. Add a "fake" usage to let the compiler know that things are as intended.

mathstuf reopened this Jul 22, 2022

mathstuf marked this pull request as draft July 22, 2022 19:29

derobins reviewed Jul 26, 2022

View reviewed changes

src/H5Oalloc.c

} /* end if */

} /* end if */

#else

(void)f;

Copy link

Member

derobins Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in develop

derobins reviewed Jul 26, 2022

View reviewed changes

derobins mentioned this pull request Jul 26, 2022

H5Tconv: fix H5T_CONV_INTERNAL_ checks #1936

Merged

derobins self-assigned this Feb 17, 2023

derobins added WIP Work in progress (please also convert PRs to draft PRs) Type - Improvement Improvements that don't add a new feature or functionality labels Mar 3, 2023

derobins reviewed Jun 7, 2023

View reviewed changes

src/H5detect.c Show resolved Hide resolved

derobins closed this Jun 7, 2023

Various fixes #1640

Various fixes #1640

Conversation

mathstuf commented Apr 14, 2022

mathstuf commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derobins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathstuf commented Apr 14, 2022

mathstuf commented Jul 22, 2022

derobins commented Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derobins commented Jun 7, 2023