areaDetector / ADCore

The home of the core components of the EPICS areaDetector software. It includes base classes for drivers and code for all of the standard plugins.
https://areadetector.github.io/master/index.html
Other
20 stars 65 forks source link

Possible error in NDFileHDF5.cpp when writing HDF5 string attributes and datasets #460

Closed MarkRivers closed 3 years ago

MarkRivers commented 3 years ago

The following error message was reported when using ADCore R3-9.

HDF5-DIAG: Error detected in HDF5 (1.10.1) thread 139745761855232:
  #000: ../H5T.c line 2153 in H5Tset_size(): size must be positive
    major: Invalid arguments to routine
    minor: Bad value

H5Tset_size() is called 3 times in NDFileHDF5.cpp:

corvette:ADCore/ADApp/pluginSrc>grep -n -U3 H5Tset_size NDFileHDF5.cpp
840-
841-  hdfdataspace     = H5Screate_simple(rank, &dims, NULL);
842-  hdfdatatype      = H5Tcopy(H5T_C_S1);
843:  hdfstatus        = H5Tset_size(hdfdatatype, str_value.size());
844-  hdfstatus        = H5Tset_strpad(hdfdatatype, H5T_STR_NULLTERM);
845-  hdfdset          = H5Dcreate2(element, name.c_str(), hdfdatatype, hdfdataspace, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT);
846-  if (hdfdset < 0) {
--
1084-
1085-  hdfattrdataspace = H5Screate(H5S_SCALAR);
1086-  hdfdatatype      = H5Tcopy(H5T_C_S1);
1087:  hdfstatus        = H5Tset_size(hdfdatatype, str_attr_value.size());
1088-  hdfstatus        = H5Tset_strpad(hdfdatatype, H5T_STR_NULLTERM);
1089-  hdfattr          = H5Acreate2(element, attr_name.c_str(), hdfdatatype, hdfattrdataspace, H5P_DEFAULT, H5P_DEFAULT);
1090-  if (hdfattr < 0) {
--
3017-  if (strlen(attrStrValue) > 0){
3018-    hdfattrdataspace = H5Screate(H5S_SCALAR);
3019-    hdfdatatype      = H5Tcopy(H5T_C_S1);
3020:    H5Tset_size(hdfdatatype, strlen(attrStrValue));
3021-    H5Tset_strpad(hdfdatatype, H5T_STR_NULLTERM);
3022-    hdfattr          = H5Acreate2(element, attrName,
3023-                                  hdfdatatype, hdfattrdataspace,

So it is passing the actual length of the string, either from std::string.size() or strlen(). However, the documentation for the H5Tset_size() functions says this:

H5Tset_size sets the total size in bytes, size, for a datatype. The parameter size must have a positive value, unless it is passed as H5T_VARIABLE and the datatype is a string datatype. String or character datatypes: The size set for a string datatype should include space for the null-terminator character, otherwise it will not be stored on (or retrieved from) disk. Adjusting the size of a string automatically sets the precision to 8*size.

Note that the datatype we are creating is H5T_C_S1 which is a C string. The documentation says that we should be passing a size that includes space for the null-terminator character, but we are not. This would explain the error message above.

This seems like a bug that should be fixed, but it will result in HDF5 files that are different from the ones we have been producing.

MarkRivers commented 3 years ago

I have reproduced this error. It only occurs for "constant" string datasets, not for string datasets that are created from NDArray string attributes. The latter are always created with a size of MAX_ATTRIBUTE_STRING_SIZE=256.

The following line in the HDF5 layout XML file will generate the error message: <dataset name="det_name" source="constant" value="" type="string" when="OnFileOpen">

The error message is:

HDF5-DIAG: Error detected in HDF5 (1.10.1) thread 139977620715264:
  #000: ../H5T.c line 2153 in H5Tset_size(): size must be positive
    major: Invalid arguments to routine
    minor: Bad value
GDYendell commented 3 years ago

It only occurs for "constant" string datasets

What is the context of constant here? Is there an example in the code?

MarkRivers commented 3 years ago

Sorry, my previous comment illustrated what I meant by "constant", but did not show up correctly because I did not escape the <dataset ... code. I have edited the comment to fix that. So it happens for datasets with source=constant and value="", i.e. a zero length string. I believe it would also happen for zero length HDF5 attributes, but I don't have an easy way to create those without modifying the code.

MarkRivers commented 3 years ago

I have created PR #461 to fix this issue.

MarkRivers commented 3 years ago

Closed via #461