Re: Test failures with latest rhdf5
Hi Mike,
thanks for the patch. I can confirm it works and is uploaded.
Kind regards, Andreas.
On Wed, Dec 18, 2019 at 06:39:18PM +0100, Mike Smith wrote:
> 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"));
--
http://fam-tille.de
Reply to: