Re: Test failures with latest rhdf5
Hi Andreas,
Attached should be diff that you can apply against /rhdf5/src/h5ls.c
I've made a few other changes since release, but I think this should be
sufficient to test whether this specific problem is fixed.
Cheers,
Mike
On 12/13/19 3:33 PM, Andreas Tille wrote:
Hi Mike,
On Fri, Dec 13, 2019 at 02:43:42PM +0100, Mike Smith wrote:
Hi Andreas,
I think I've narrowed this down to an unsafe use of sprintf() in that
function.
There's a modified version of the package at
https://github.com/grimbough/rhdf5/tree/h5ls_printing
I haven't been able to reproduce the bug on my own system, but if you'd like
to test that version and report back, that would be great.
I'm not sure what format you need for testing, so if you'd like me to
provide the source tarball or anything let me know.
The easiest way to verify your changes is a patch against the released
version. I admit I tried a diff between the whole directories but this
is quite large and does not really fit to a simple change in an
sprintf().
So a small diff would be very convenient.
Kind regards and thanks for your support
Andreas.
On 12/6/19 10:01 AM, Andreas Tille wrote:
Hi Mike,
thanks a lot for the speedy response!
On Fri, Dec 06, 2019 at 08:06:44AM +0000, Mike Smith wrote:
Hi Andreas,
Thanks for the heads up. That's a weird error because really it's just comparing too strings that represent the shape of the datasets, so it's the string creation rather than anything else that's failing.
This was reported once before (https://support.bioconductor.org/p/101038/ (https://support.bioconductor.org/p/101038/)) but I put it down to using old versions of R/rhdf5 that predated my time as maintainer, and the user never got back to me with more details.
I presume you have built many previous versions of rhdf5 on your system, and I haven't changed the h5ls() function in years, so it's odd that it's manifesting now.
I need to admit that also the previous version 2.30.0 showed the exact
same issue. However, non of the maintainers team payed sufficient
attention to that issue.
I'll take a look at the code and see if there's anything wrong in h5ls(), I guess this must be a pretty weird edge case if it doesn't show up anywhere else.
I'll let you know if I find something.
Thanks a lot again for your quick response. Feel free to ask here if
you need some further information.
Kind regards
Andreas.
Cheers,
Mike
On Fri, Dec 6, 2019 at 08:35, Andreas Tille wrote:
Hi Mike,
I intent to upgrade the Debian package of rhdf5 to version 2.30.1
but I stumbled about the following issues in the test suite:
library(testthat)
library(rhdf5)
.
test_check("rhdf5")
── 1. Failure: h5ls supports native (@test_native.R#21) ───────────────────────
`object` not equal to "3 x 4".
1/1 mismatches
x[1]: " x 4"
y[1]: "3 x 4"
── 2. Failure: h5ls supports native (@test_native.R#23) ───────────────────────
`object` not equal to "4 x 3".
1/1 mismatches
x[1]: " x 3"
y[1]: "4 x 3"
── 3. Failure: h5ls supports native (@test_native.R#27) ───────────────────────
`object` not equal to "4 x 3".
1/1 mismatches
x[1]: " x 3"
y[1]: "4 x 3"
── 4. Failure: h5ls supports native (@test_native.R#29) ───────────────────────
`object` not equal to "3 x 4".
1/1 mismatches
x[1]: " x 4"
y[1]: "3 x 4"
── 5. Failure: h5ls supports native (@test_native.R#38) ───────────────────────
`object` not equal to "2 x 3 x 4".
1/1 mismatches
x[1]: " x 4"
y[1]: "2 x 3 x 4"
── 6. Failure: h5ls supports native (@test_native.R#41) ───────────────────────
`object` not equal to "4 x 3 x 2".
1/1 mismatches
x[1]: " x 2"
y[1]: "4 x 3 x 2"
── 7. Failure: h5ls supports native (@test_native.R#46) ───────────────────────
`object` not equal to "4 x 3 x 2".
1/1 mismatches
x[1]: " x 2"
y[1]: "4 x 3 x 2"
── 8. Failure: h5ls supports native (@test_native.R#49) ───────────────────────
`object` not equal to "2 x 3 x 4".
1/1 mismatches
x[1]: " x 4"
y[1]: "2 x 3 x 4"
══ testthat results ═══════════════════════════════════════════════════════════
[ OK: 694 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 8 ]
1. Failure: h5ls supports native (@test_native.R#21).
2. Failure: h5ls supports native (@test_native.R#23).
3. Failure: h5ls supports native (@test_native.R#27).
4. Failure: h5ls supports native (@test_native.R#29).
5. Failure: h5ls supports native (@test_native.R#38).
6. Failure: h5ls supports native (@test_native.R#41).
7. Failure: h5ls supports native (@test_native.R#46).
8. Failure: h5ls supports native (@test_native.R#49).
Error: testthat unit tests failed
Execution halted
Do you have any idea what might be wrong here?
Kind regards
Andreas.
--
http://fam-tille.de (http://fam-tille.de)
diff --git a/src/h5ls.c b/src/h5ls.c
index cef3ee0..c22527c 100644
--- a/src/h5ls.c
+++ b/src/h5ls.c
@@ -3,6 +3,33 @@
#include <stdio.h>
#include "printdatatype.h"
+#include <stdlib.h>
+#include <string.h>
+
+void concat(char *s1, hsize_t next_dim, int index)
+{
+ char tmp[100];
+ strncpy(tmp, s1, 100);
+
+#ifdef H5_HAVE_WINDOWS
+ snprintf(s1, 100, "%s%I64u%s", tmp, next_dim, index?" x ":"");
+#else
+ snprintf(s1, 100, "%s%llu%s", tmp, next_dim, index ? " x " : "");
+#endif
+}
+
+void concatnative(char *s1, hsize_t next_dim, int index)
+{
+ char tmp[100];
+ strncpy(tmp, s1, 100);
+
+#ifdef H5_HAVE_WINDOWS
+ snprintf(s1, 100, "%s%s%I64u", tmp, index?" x ":"", next_dim);
+#else
+ snprintf(s1, 100, "%s%s%llu", tmp, index ? " x " : "", next_dim);
+#endif
+}
+
typedef struct opLinfoListElement {
long idx;
char *name;
@@ -16,7 +43,6 @@ typedef struct opLinfoListElement {
H5L_info_t info;
H5I_type_t type;
hsize_t num_attrs;
- // H5O_info_t object_info;
struct opLinfoListElement *next;
} opLinfoListElement;
@@ -39,7 +65,6 @@ herr_t opAddToLinfoList( hid_t g_id, const char *name, const H5L_info_t *info, v
herr_t herr = 0;
opLinfoListElement *newElement = (opLinfoListElement *)R_alloc(1,sizeof(struct opLinfoListElement) );
newElement->idx = data->n;
- // printf("sizeof = %ld cset=%ld group=>%s< name=>%s<\n", strlen(name), info->cset, data->group, name);
newElement->name = (char *)R_alloc(1,(strlen(name)+1)*sizeof(char));
strcpy(newElement->name, name);
newElement->group = (char *)R_alloc(1,(strlen(data->group)+1)*sizeof(char));
@@ -66,7 +91,6 @@ herr_t opAddToLinfoList( hid_t g_id, const char *name, const H5L_info_t *info, v
data->last = newElement;
} else {
hid_t oid = H5Oopen( g_id, name, H5P_DEFAULT );
- // herr_t herr = H5Oget_info( oid, &(newElement->object_info) );
newElement->type = H5Iget_type(oid);
newElement->num_attrs = H5Oget_num_attrs(oid);
if ((data->showdatasetinfo) & (newElement->type == H5I_DATASET)) {
@@ -74,7 +98,6 @@ herr_t opAddToLinfoList( hid_t g_id, const char *name, const H5L_info_t *info, v
hid_t type = H5Dget_type(did);
newElement->datatype = getDatatypeName(type);
newElement->class = getDatatypeClass(type);
- /* H5Tclose(type); */
hid_t sid = H5Dget_space( did );
hsize_t size[H5S_MAX_RANK];
hsize_t maxsize[H5S_MAX_RANK];
@@ -97,67 +120,31 @@ herr_t opAddToLinfoList( hid_t g_id, const char *name, const H5L_info_t *info, v
} break;
case H5S_SIMPLE: {
char* tmp = (char *)R_alloc(100*newElement->rank,sizeof(char));
+ memset(tmp, '\0', 100 * sizeof(char));
if (data->native) {
-#ifdef H5_HAVE_WINDOWS
- sprintf(tmp, "%I64u", size[0]);
-#else
- sprintf(tmp, "%llu", size[0]);
-#endif
- for(int i = 1; i < newElement->rank; i++) {
-#ifdef H5_HAVE_WINDOWS
- sprintf(tmp, "%s x %I64u", tmp, size[i]);
-#else
- sprintf(tmp, "%s x %llu", tmp, size[i]);
-#endif
+ //TODO: can we end up here?
+ for(int i = 0; i < newElement->rank; i++) {
+ concatnative(tmp, size[i], i);
}
} else {
-#ifdef H5_HAVE_WINDOWS
- sprintf(tmp, "%I64u", size[newElement->rank-1]);
-#else
- sprintf(tmp, "%llu", size[newElement->rank-1]);
-#endif
- for(int i = newElement->rank-2; i >= 0; i--) {
-#ifdef H5_HAVE_WINDOWS
- sprintf(tmp, "%s x %I64u", tmp, size[i]);
-#else
- sprintf(tmp, "%s x %llu", tmp, size[i]);
-#endif
+ for(int i = newElement->rank-1; i >= 0; i--) {
+ concat(tmp, size[i], i);
}
}
- sprintf(tmp, "%s", tmp);
newElement->dim = (char *)R_alloc((strlen(tmp)+1),sizeof(char));
strcpy(newElement->dim, tmp);
+
if(maxsize[0] == H5S_UNLIMITED) {
sprintf(tmp, "UNLIMITED");
} else {
+ memset(tmp, '\0', 100 * sizeof(char));
if (data->native) {
-#ifdef H5_HAVE_WINDOWS
- sprintf(tmp, "%I64u", maxsize[0]);
-#else
- sprintf(tmp, "%llu", maxsize[0]);
-#endif
- for(int i = 1; i < newElement->rank ; i++) {
-#ifdef H5_HAVE_WINDOWS
- sprintf(tmp, "%s x %I64u", tmp, maxsize[i]);
-#else
- sprintf(tmp, "%s x %llu", tmp, maxsize[i]);
-#endif
- }
+ //TODO: can we end up here?
} else {
-#ifdef H5_HAVE_WINDOWS
- sprintf(tmp, "%I64u", maxsize[newElement->rank-1]);
-#else
- sprintf(tmp, "%llu", maxsize[newElement->rank-1]);
-#endif
- for(int i = newElement->rank-2; i >= 0 ; i--) {
-#ifdef H5_HAVE_WINDOWS
- sprintf(tmp, "%s x %I64u", tmp, maxsize[i]);
-#else
- sprintf(tmp, "%s x %llu", tmp, maxsize[i]);
-#endif
+ for(int i = 0; i < newElement->rank; i++) {
+ concat(tmp, maxsize[i], i);
}
}
- sprintf(tmp, "%s", tmp);
}
newElement->maxdim = (char *)R_alloc((strlen(tmp)+1),sizeof(char));
strcpy(newElement->maxdim, tmp);
@@ -172,17 +159,8 @@ herr_t opAddToLinfoList( hid_t g_id, const char *name, const H5L_info_t *info, v
newElement->maxdim = "unknown dataspace";
} break;
} /* end switch */
- H5Sclose(sid);
+ H5Sclose(sid);
- /* printf("type=%ld\n",H5T_STD_I32LE); */
- /* printf("type=%ld\n",H5T_IEEE_F32LE); */
- /* const char *typename = getDatatypeName(type, 1); */
- /* printf("type=%ld\n",hid); */
- /* char *typename; */
- /* typename = malloc(1001*sizeof(char)); */
- /* ssize_t s = H5Iget_name( hid, typename, 1000 ); */
- /* printf("size=%ld\n",s); */
- /* printf("name=%s\n\n",typename); */
H5Dclose(did);
} else {
newElement->datatype = "";
@@ -227,7 +205,7 @@ herr_t opAddToLinfoList( hid_t g_id, const char *name, const H5L_info_t *info, v
}
SEXP _h5ls( SEXP _loc_id, SEXP _depth, SEXP _datasetinfo, SEXP _index_type, SEXP _order, SEXP _native ) {
- //hid_t loc_id = INTEGER(_loc_id)[0];
+
hid_t loc_id = STRSXP_2_HID( _loc_id );
opLinfoList data;
data.n = 0;
@@ -241,10 +219,7 @@ SEXP _h5ls( SEXP _loc_id, SEXP _depth, SEXP _datasetinfo, SEXP _index_type, SEXP
data.last = NULL;
data.index_type = INTEGER(_index_type)[0];
data.order = INTEGER(_order)[0];
- // H5_index_t index_type = H5_INDEX_NAME;
- // H5_iter_order_t order = H5_ITER_INC;
hsize_t idx=0;
- // printf("Start visit.\n");
herr_t herr = H5Literate( loc_id, data.index_type, data.order, &idx, &opAddToLinfoList, &data );
@@ -256,21 +231,13 @@ SEXP _h5ls( SEXP _loc_id, SEXP _depth, SEXP _datasetinfo, SEXP _index_type, SEXP
UNPROTECT(1);
} else {
PROTECT(Rval= allocVector(VECSXP, 14));
- // SEXP elementnames = PROTECT(allocVector(STRSXP, 0));
SEXP group = PROTECT(allocVector(STRSXP, data.n));
SEXP elementnames = PROTECT(allocVector(STRSXP, data.n));
SEXP ltype = PROTECT(allocVector(INTSXP, data.n));
SEXP corder_valid = PROTECT(allocVector(LGLSXP, data.n));
SEXP corder = PROTECT(allocVector(INTSXP, data.n));
SEXP cset = PROTECT(allocVector(INTSXP, data.n));
- // SEXP fileno = PROTECT(allocVector(INTSXP, data.n));
- // SEXP addr = PROTECT(allocVector(INTSXP, data.n));
SEXP otype = PROTECT(allocVector(INTSXP, data.n));
- // SEXP rc = PROTECT(allocVector(INTSXP, data.n));
- // SEXP atime = PROTECT(allocVector(REALSXP, data.n));
- // SEXP mtime = PROTECT(allocVector(REALSXP, data.n));
- // SEXP ctime = PROTECT(allocVector(REALSXP, data.n));
- // SEXP btime = PROTECT(allocVector(REALSXP, data.n));
SEXP num_attrs = PROTECT(allocVector(INTSXP, data.n));
SEXP dclass = PROTECT(allocVector(STRSXP, data.n));
SEXP dtype = PROTECT(allocVector(STRSXP, data.n));
@@ -283,21 +250,13 @@ SEXP _h5ls( SEXP _loc_id, SEXP _depth, SEXP _datasetinfo, SEXP _index_type, SEXP
opLinfoListElement *el = data.first;
opLinfoListElement *elnext;
while (el != NULL) {
- // printf("element %d\n",el->idx);
SET_STRING_ELT(group, el->idx, mkChar(el->group));
SET_STRING_ELT(elementnames, el->idx, mkChar(el->name));
INTEGER(ltype)[el->idx] = el->info.type;
LOGICAL(corder_valid)[el->idx] = el->info.corder_valid;
INTEGER(corder)[el->idx] = el->info.corder;
INTEGER(cset)[el->idx] = el->info.cset;
- // INTEGER(fileno)[el->idx] = el->object_info.fileno;
- // INTEGER(addr)[el->idx] = el->object_info.addr;
INTEGER(otype)[el->idx] = el->type;
- // INTEGER(rc)[el->idx] = el->object_info.rc;
- // REAL(atime)[el->idx] = el->object_info.atime;
- // REAL(mtime)[el->idx] = el->object_info.mtime;
- // REAL(ctime)[el->idx] = el->object_info.ctime;
- // REAL(btime)[el->idx] = el->object_info.btime;
INTEGER(num_attrs)[el->idx] = el->num_attrs;
SET_STRING_ELT(dclass, el->idx, mkChar(el->class));
SET_STRING_ELT(dtype, el->idx, mkChar(el->datatype));
@@ -316,14 +275,7 @@ SEXP _h5ls( SEXP _loc_id, SEXP _depth, SEXP _datasetinfo, SEXP _index_type, SEXP
SET_VECTOR_ELT(Rval,3,corder_valid);
SET_VECTOR_ELT(Rval,4,corder);
SET_VECTOR_ELT(Rval,5,cset);
- // SET_VECTOR_ELT(Rval,6,fileno);
- // SET_VECTOR_ELT(Rval,7,addr);
SET_VECTOR_ELT(Rval,6,otype);
- // SET_VECTOR_ELT(Rval,9,rc);
- // SET_VECTOR_ELT(Rval,10,atime);
- // SET_VECTOR_ELT(Rval,11,mtime);
- // SET_VECTOR_ELT(Rval,12,ctime);
- // SET_VECTOR_ELT(Rval,13,btime);
SET_VECTOR_ELT(Rval,7,num_attrs);
SET_VECTOR_ELT(Rval,8,dclass);
SET_VECTOR_ELT(Rval,9,dtype);
@@ -339,14 +291,7 @@ SEXP _h5ls( SEXP _loc_id, SEXP _depth, SEXP _datasetinfo, SEXP _index_type, SEXP
SET_STRING_ELT(names, 3, mkChar("corder_valid"));
SET_STRING_ELT(names, 4, mkChar("corder"));
SET_STRING_ELT(names, 5, mkChar("cset"));
- // SET_STRING_ELT(names, 6, mkChar("fileno"));
- // SET_STRING_ELT(names, 7, mkChar("addr"));
SET_STRING_ELT(names, 6, mkChar("otype"));
- // SET_STRING_ELT(names, 9, mkChar("rc"));
- // SET_STRING_ELT(names, 10, mkChar("atime"));
- // SET_STRING_ELT(names, 11, mkChar("mtime"));
- // SET_STRING_ELT(names, 12, mkChar("ctime"));
- // SET_STRING_ELT(names, 13, mkChar("btime"));
SET_STRING_ELT(names, 7, mkChar("num_attrs"));
SET_STRING_ELT(names, 8, mkChar("dclass"));
SET_STRING_ELT(names, 9, mkChar("dtype"));
Reply to: