Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unit_test_tst_exhash performs a too-large left shift #2702

Closed
seanm opened this issue May 25, 2023 · 4 comments
Closed

unit_test_tst_exhash performs a too-large left shift #2702

seanm opened this issue May 25, 2023 · 4 comments

Comments

@seanm
Copy link
Contributor

seanm commented May 25, 2023

  1. build master with UBSan
  2. run ctest -R unit_test_tst_exhash
  3. UBSan will detect a too-large left shift, see my lldb session:
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
    frame #0: 0x0000000100a42dc3 libnetcdf.19.dylib`ncexhashprintstats(map=0x0000606000000e60) at ncexhash.c:857:20
   854 	    fprintf(stderr," |leaf|=%d nactive/nleaves=%g", map->leaflen, leafavg);
   855 	    fprintf(stderr," load=%g",leafload);
   856 	    fprintf(stderr,"]\n");
-> 857 	    dirsize = (1ULL<<(map->depth)*((unsigned long long)sizeof(void*)));
   858 	    leafsize = (nleaves)*((unsigned long long)sizeof(NCexleaf));
   859 	    total = dirsize + leafsize;
   860 	    fprintf(stderr,"\tsizeof(directory)=%llu sizeof(leaves)=%lld total=%lld\n",

(lldb) p map->depth
(int) $0 = 11

(lldb) p sizeof(void*)
(unsigned long) $1 = 8

So basically it's doing 1ULL << 88.

Not sure what value it's expecting to get with such an operation, but it's undefined behaviour to shift into, or past, the MSB.

@DennisHeimbigner
Copy link
Collaborator

DennisHeimbigner commented May 25, 2023

Looks like I am missing a set of parentheses.
I suspect it should have been:

dirsize = ((1ULL<<(map->depth))*((unsigned long long)sizeof(void*)));

@seanm
Copy link
Contributor Author

seanm commented May 25, 2023

Could be. I have no idea. :) I changed it to dirsize = (1ULL << map->depth) * sizeof(void*); and the test now passes, but that doesn't prove the change is correct of course...

@seanm
Copy link
Contributor Author

seanm commented May 25, 2023

At least I think that's what you were suggesting... your asterixes got interpreted as markdown...

DennisHeimbigner added a commit to DennisHeimbigner/netcdf-c that referenced this issue Jul 24, 2023
1. Fix a shift bug in ncexhash.c (Issue Unidata#2702)
2. Fix an S3 related error in test_byterange.sh
3. Fix bz2/bzip2 handling in configure.ac
@DennisHeimbigner
Copy link
Collaborator

Fixed by PR #2726.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants