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

Clang warnings and ASan fixes #3

Closed
wants to merge 8 commits into from

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Jun 29, 2020

This fixes a few compiler warnings and ASan reports.

seanm added 8 commits June 29, 2020 12:04
Either included missing header or added a prototype.
…inator

Found by ASan:

==18515==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200001a791 at pc 0x00010111b47a bp 0x7ffeefbfe8f0 sp 0x7ffeefbfe098
READ of size 2 at 0x60200001a791 thread T0
    #0 0x10111b479 in wrap_strlen (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x14479)
    HDFGroup#1 0x100841c4c in H5T__conv_vlen H5Tconv.c:3193
    HDFGroup#2 0x1008113d9 in H5T_convert H5T.c:5024
    HDFGroup#3 0x100379fbb in H5D__scatgath_write H5Dscatgath.c:701
    HDFGroup#4 0x10032d235 in H5D__contig_write H5Dcontig.c:657
    HDFGroup#5 0x10036c768 in H5D__write H5Dio.c:819
    HDFGroup#6 0x10036b650 in H5Dwrite H5Dio.c:335
    HDFGroup#7 0x100115fa7 in H5::DataSet::write(void const*, H5::DataType const&, H5::DataSpace const&, H5::DataSpace const&, H5::DSetMemXferPropList const&) const H5DataSet.cpp:506
    HDFGroup#8 0x100031d9a in test_vlstrings tvlstr.cpp:191
    HDFGroup#9 0x1001aec42 in PerformTests testframe.c:323
    HDFGroup#10 0x100002ac0 in main testhdf5.cpp:116
    HDFGroup#11 0x7fff5eec6014 in start (libdyld.dylib:x86_64+0x1014)

0x60200001a791 is located 0 bytes to the right of 1-byte region [0x60200001a790,0x60200001a791)
allocated by thread T0 here:
    #0 0x10115c547 in wrap_calloc (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x55547)
    HDFGroup#1 0x100031d70 in test_vlstrings tvlstr.cpp:187
    HDFGroup#2 0x1001aec42 in PerformTests testframe.c:323
    HDFGroup#3 0x100002ac0 in main testhdf5.cpp:116
    HDFGroup#4 0x7fff5eec6014 in start (libdyld.dylib:x86_64+0x1014)
…H5Rdereference2()

Found by ASan:

==52756==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffeefbfedc8 at pc 0x000100be250f bp 0x7ffeefbfe890 sp 0x7ffeefbfe888
READ of size 1 at 0x7ffeefbfedc8 thread T0
    #0 0x100be250e in H5R__dereference H5Rint.c:416
    HDFGroup#1 0x100bddbd1 in H5Rdereference2 H5R.c:185
    HDFGroup#2 0x1001c168c in test_reference_region trefer.c:798
    HDFGroup#3 0x1001ba134 in test_reference trefer.c:1863
    HDFGroup#4 0x100660c42 in PerformTests testframe.c:323
    HDFGroup#5 0x100001fdc in main testhdf5.c:77
    HDFGroup#6 0x7fff5eec6014 in start (libdyld.dylib:x86_64+0x1014)

Address 0x7ffeefbfedc8 is located in stack of thread T0 at offset 616 in frame
    #0 0x1001be1df in test_reference_region trefer.c:507
… for the actual data

Found by ASan:

==19994==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffeefbfe920 at pc 0x0001010b064f bp 0x7ffeefbfc810 sp 0x7ffeefbfbfc0
READ of size 128 at 0x7ffeefbfe920 thread T0
    #0 0x1010b064e in __asan_memcpy (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x5264e)
    HDFGroup#1 0x1002cde61 in H5D__gather_mem H5Dscatgath.c:428
    HDFGroup#2 0x1002d0ca4 in H5D__scatgath_write H5Dscatgath.c:663
    HDFGroup#3 0x10025c948 in H5D__chunk_write H5Dchunk.c:2470
    HDFGroup#4 0x1002c3768 in H5D__write H5Dio.c:819
    HDFGroup#5 0x1002c2650 in H5Dwrite H5Dio.c:335
    HDFGroup#6 0x1000d4b8a in H5TBwrite_fields_index H5TB.c:729
    HDFGroup#7 0x100008c13 in test_table test_table.c:1420
    HDFGroup#8 0x1000028ff in main test_table.c:1686
    HDFGroup#9 0x7fff5eec6014 in start (libdyld.dylib:x86_64+0x1014)

Address 0x7ffeefbfe920 is located in stack of thread T0 at offset 4160 in frame
    #0 0x1000029cf in test_table test_table.c:211
@epourmal
Copy link

epourmal commented Jul 1, 2020

Sean,

Thank you so much for your contribution! This repo is not open for contributions yet. We will be announcing our move from Bitbucket to Github before the end of July.

Could you please submit your patch against https://bitbucket.hdfgroup.org/projects/HDFFV/repos/hdf5/browse and we will review and merge, or could you please wait until we make an announcement?

Our repo doesn't have standard git organization and all changes should be done to develop branch first and then we will propagate the changes to the other maintenance branches.

Thank you!

Elena

@seanm
Copy link
Contributor Author

seanm commented Jul 1, 2020

The only one I really need ASAP is 29e6401, because it prevents compilation on the beta of macOS 11 Big Sur because the -Wimplicit-function-declaration warning is now treated as an error.

Building the 'develop' branch doesn't seem to have those warnings. I guess someone fixed them already. Given that, there's no change to the 'develop' branch I can even submit. How to proceed? Thanks.

@lrknox
Copy link
Collaborator

lrknox commented Jul 1, 2020

Hi Sean,

I checked for the 29e6401 changes in our 1.10 development branch hdf5_1_10, and they are in that branch. I suspect that if the rest of these changes are in develop, they are also in the hdf5_1_10 branch. we currently reserve the 1.x/master branches for committing official releases. The code currently in hdf5_1_10 with another month's work should be posted there sometime in August. If the changes you need are in that branch, can you use the branch? I can also create a snapshot release from that branch this weekend, and if the branch doesn't have all of your changes we can review them and try to get them into the snapshot.

Larry

@seanm
Copy link
Contributor Author

seanm commented Feb 4, 2021

This repo is not open for contributions yet.

Is it now?

I've just updated myself to c55ac8a on develop branch and 3 tests fail with address sanitizer, specifically:

	 26 - H5TEST-dsets (Failed)
	2082 - HL_test_table (Failed)
	2107 - CPP_testhdf5 (Failed)

Probably the same 3 as in my patch...

@byrnHDF
Copy link
Contributor

byrnHDF commented Feb 4, 2021

I have implemented some of these changes (not the CX includes yet) that are applicable.
It has fixed two tests; CPP_testhdf5 and HL_test_table.

@seanm
Copy link
Contributor Author

seanm commented Feb 4, 2021

Great, thanks. But where? I don't see it here: https://github.com/HDFGroup/hdf5/commits/develop am I looking in the wrong place?

@byrnHDF
Copy link
Contributor

byrnHDF commented Feb 4, 2021

Still on my local machine - need to create PRs.
Need to look into the rest.

@seanm
Copy link
Contributor Author

seanm commented Feb 4, 2021

Ah, I see. Can this PR not be used?

@byrnHDF
Copy link
Contributor

byrnHDF commented Feb 4, 2021

Ah, I see. Can this PR not be used?

Right, this PR is against 1.10/master which is "release tagging" branch. The normal process is develop, to 1.12, to 1.10, to 1.8.
I will have 4 PRs up soon after testing is verified.

@byrnHDF
Copy link
Contributor

byrnHDF commented Feb 4, 2021

PR #312 , #313 , #314 and #315 created for these changes.

@seanm
Copy link
Contributor Author

seanm commented Feb 6, 2021

Great thanks!

@seanm
Copy link
Contributor Author

seanm commented Feb 14, 2021

So I can confirm that two are fixed, and H5TEST-dsets is not. I debugged it a bit:

H5MM_memcpy is reading past the end of a 32 byte buffer because it's being told to read 256 bytes. The buffer is:

hdf5/test/dsets.c

Line 12185 in 52ac746

const int buffer[8] = {0, 1, 2, 3, 4, 5, 6, 7};

The backtrace is:

frame #5: 0x0000000101490c5c libhdf5_debug.1000.dylib`H5MM_memcpy(dest=0x00006120004816c8, src=0x00000001000f7120, n=256) at H5MM.c:606:11
frame #6: 0x00000001026ddbd1 libhdf5_debug.1000.dylib`H5VM_memcpyvv(_dst=0x00006120004816c8, dst_max_nseq=1, dst_curr_seq=0x00007ffeefbfbae0, dst_len_arr=0x0000625000005108, dst_off_arr=0x0000625000007908, _src=0x00000001000f7120, src_max_nseq=1, src_curr_seq=0x00007ffeefbfbac0, src_len_arr=0x0000625000000108, src_off_arr=0x0000625000002908) at H5VM.c:1625:13
frame #7: 0x00000001009c0b83 libhdf5_debug.1000.dylib`H5D__compact_writevv(io_info=0x00007ffeefbfc570, dset_max_nseq=1, dset_curr_seq=0x00007ffeefbfbae0, dset_size_arr=0x0000625000005108, dset_offset_arr=0x0000625000007908, mem_max_nseq=1, mem_curr_seq=0x00007ffeefbfbac0, mem_size_arr=0x0000625000000108, mem_offset_arr=0x0000625000002908) at H5Dcompact.c:314:22
frame #8: 0x0000000100b1634a libhdf5_debug.1000.dylib`H5D__select_io(io_info=0x00007ffeefbfc570, elmt_size=4, nelmts=64, file_space=0x0000613000b5fe80, mem_space=0x000061300053afc0) at H5Dselect.c:221:37
frame #9: 0x0000000100b172b3 libhdf5_debug.1000.dylib`H5D__select_write(io_info=0x00007ffeefbfc570, type_info=0x00007ffeefbfd110, nelmts=64, file_space=0x0000613000b5fe80, mem_space=0x000061300053afc0) at H5Dselect.c:310:9
frame #10: 0x00000001008fdb49 libhdf5_debug.1000.dylib`H5D__chunk_write(io_info=0x00007ffeefbfd020, type_info=0x00007ffeefbfd110, nelmts=64, file_space=0x000061300053afc0, mem_space=0x000061300053afc0, fm=0x0000620000000080) at H5Dchunk.c:2730:13
frame #11: 0x0000000100ae05be libhdf5_debug.1000.dylib`H5D__write(dataset=0x0000606000013580, mem_type_id=216172782113784762, mem_space=0x000061300053afc0, file_space=0x000061300053afc0, buf=0x00000001000f7120) at H5Dio.c:531:9
frame #12: 0x000000010265d946 libhdf5_debug.1000.dylib`H5VL__native_dataset_write(obj=0x0000606000013580, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=792633534417207304, buf=0x00000001000f7120, req=0x0000000000000000) at H5VLnative_dataset.c:206:9
frame #13: 0x00000001025aebea libhdf5_debug.1000.dylib`H5VL__dataset_write(obj=0x0000606000013580, cls=0x0000616000000980, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=792633534417207304, buf=0x00000001000f7120, req=0x0000000000000000) at H5VLcallback.c:2082:9
frame #14: 0x00000001025ae050 libhdf5_debug.1000.dylib`H5VL_dataset_write(vol_obj=0x0000603000006640, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=792633534417207304, buf=0x00000001000f7120, req=0x0000000000000000) at H5VLcallback.c:2114:9
frame #15: 0x000000010088d551 libhdf5_debug.1000.dylib`H5D__write_api_common(dset_id=360287970189640115, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=792633534417207304, buf=0x00000001000f7120, token_ptr=0x0000000000000000, _vol_obj_ptr=0x0000000000000000) at H5D.c:1105:9
frame #16: 0x000000010088c561 libhdf5_debug.1000.dylib`H5Dwrite(dset_id=360287970189640115, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=0, buf=0x00000001000f7120) at H5D.c:1154:9
frame #17: 0x00000001000b856b dsets`test_bt2_hdr_fd(env_h5_driver="nomatch", fapl=792633534417208475) at dsets.c:12488:9
frame #18: 0x0000000100007390 dsets`main at dsets.c:15270:29
(lldb) fr sel 6
frame #6: 0x00000001026ddbd1 libhdf5_debug.1000.dylib`H5VM_memcpyvv(_dst=0x00006120004816c8, dst_max_nseq=1, dst_curr_seq=0x00007ffeefbfbae0, dst_len_arr=0x0000625000005108, dst_off_arr=0x0000625000007908, _src=0x00000001000f7120, src_max_nseq=1, src_curr_seq=0x00007ffeefbfbac0, src_len_arr=0x0000625000000108, src_off_arr=0x0000625000002908) at H5VM.c:1625:13
   1622	        acc_len = 0;
   1623	        do {
   1624	            /* Copy data */
-> 1625	            H5MM_memcpy(dst, src, tmp_dst_len);
   1626	
   1627	            /* Accumulate number of bytes copied */
   1628	            acc_len += tmp_dst_len;

(lldb) p tmp_dst_len
(size_t) $11 = 256

(lldb) p tmp_src_len
(size_t) $12 = 256

I gave up at that point, but hopefully it gives you a head start.

@seanm
Copy link
Contributor Author

seanm commented Feb 14, 2021

So I can confirm that two are fixed, and H5TEST-dsets is not. I debugged it a bit:

H5MM_memcpy is reading past the end of a 32 byte buffer because it's being told to read 256 bytes. The buffer is:

hdf5/test/dsets.c

Line 12185 in 52ac746

const int buffer[8] = {0, 1, 2, 3, 4, 5, 6, 7};

const int buffer[8] = {0, 1, 2, 3, 4, 5, 6, 7};

The backtrace is:

frame #5: 0x0000000101490c5c libhdf5_debug.1000.dylib`H5MM_memcpy(dest=0x00006120004816c8, src=0x00000001000f7120, n=256) at H5MM.c:606:11
frame #6: 0x00000001026ddbd1 libhdf5_debug.1000.dylib`H5VM_memcpyvv(_dst=0x00006120004816c8, dst_max_nseq=1, dst_curr_seq=0x00007ffeefbfbae0, dst_len_arr=0x0000625000005108, dst_off_arr=0x0000625000007908, _src=0x00000001000f7120, src_max_nseq=1, src_curr_seq=0x00007ffeefbfbac0, src_len_arr=0x0000625000000108, src_off_arr=0x0000625000002908) at H5VM.c:1625:13
frame #7: 0x00000001009c0b83 libhdf5_debug.1000.dylib`H5D__compact_writevv(io_info=0x00007ffeefbfc570, dset_max_nseq=1, dset_curr_seq=0x00007ffeefbfbae0, dset_size_arr=0x0000625000005108, dset_offset_arr=0x0000625000007908, mem_max_nseq=1, mem_curr_seq=0x00007ffeefbfbac0, mem_size_arr=0x0000625000000108, mem_offset_arr=0x0000625000002908) at H5Dcompact.c:314:22
frame #8: 0x0000000100b1634a libhdf5_debug.1000.dylib`H5D__select_io(io_info=0x00007ffeefbfc570, elmt_size=4, nelmts=64, file_space=0x0000613000b5fe80, mem_space=0x000061300053afc0) at H5Dselect.c:221:37
frame #9: 0x0000000100b172b3 libhdf5_debug.1000.dylib`H5D__select_write(io_info=0x00007ffeefbfc570, type_info=0x00007ffeefbfd110, nelmts=64, file_space=0x0000613000b5fe80, mem_space=0x000061300053afc0) at H5Dselect.c:310:9
frame #10: 0x00000001008fdb49 libhdf5_debug.1000.dylib`H5D__chunk_write(io_info=0x00007ffeefbfd020, type_info=0x00007ffeefbfd110, nelmts=64, file_space=0x000061300053afc0, mem_space=0x000061300053afc0, fm=0x0000620000000080) at H5Dchunk.c:2730:13
frame #11: 0x0000000100ae05be libhdf5_debug.1000.dylib`H5D__write(dataset=0x0000606000013580, mem_type_id=216172782113784762, mem_space=0x000061300053afc0, file_space=0x000061300053afc0, buf=0x00000001000f7120) at H5Dio.c:531:9
frame #12: 0x000000010265d946 libhdf5_debug.1000.dylib`H5VL__native_dataset_write(obj=0x0000606000013580, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=792633534417207304, buf=0x00000001000f7120, req=0x0000000000000000) at H5VLnative_dataset.c:206:9
frame #13: 0x00000001025aebea libhdf5_debug.1000.dylib`H5VL__dataset_write(obj=0x0000606000013580, cls=0x0000616000000980, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=792633534417207304, buf=0x00000001000f7120, req=0x0000000000000000) at H5VLcallback.c:2082:9
frame #14: 0x00000001025ae050 libhdf5_debug.1000.dylib`H5VL_dataset_write(vol_obj=0x0000603000006640, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=792633534417207304, buf=0x00000001000f7120, req=0x0000000000000000) at H5VLcallback.c:2114:9
frame #15: 0x000000010088d551 libhdf5_debug.1000.dylib`H5D__write_api_common(dset_id=360287970189640115, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=792633534417207304, buf=0x00000001000f7120, token_ptr=0x0000000000000000, _vol_obj_ptr=0x0000000000000000) at H5D.c:1105:9
frame #16: 0x000000010088c561 libhdf5_debug.1000.dylib`H5Dwrite(dset_id=360287970189640115, mem_type_id=216172782113784762, mem_space_id=0, file_space_id=0, dxpl_id=0, buf=0x00000001000f7120) at H5D.c:1154:9
frame #17: 0x00000001000b856b dsets`test_bt2_hdr_fd(env_h5_driver="nomatch", fapl=792633534417208475) at dsets.c:12488:9
frame #18: 0x0000000100007390 dsets`main at dsets.c:15270:29
(lldb) fr sel 6
frame #6: 0x00000001026ddbd1 libhdf5_debug.1000.dylib`H5VM_memcpyvv(_dst=0x00006120004816c8, dst_max_nseq=1, dst_curr_seq=0x00007ffeefbfbae0, dst_len_arr=0x0000625000005108, dst_off_arr=0x0000625000007908, _src=0x00000001000f7120, src_max_nseq=1, src_curr_seq=0x00007ffeefbfbac0, src_len_arr=0x0000625000000108, src_off_arr=0x0000625000002908) at H5VM.c:1625:13
   1622	        acc_len = 0;
   1623	        do {
   1624	            /* Copy data */
-> 1625	            H5MM_memcpy(dst, src, tmp_dst_len);
   1626	
   1627	            /* Accumulate number of bytes copied */
   1628	            acc_len += tmp_dst_len;

(lldb) p tmp_dst_len
(size_t) $11 = 256

(lldb) p tmp_src_len
(size_t) $12 = 256

I gave up at that point, but hopefully it gives you a head start.

@seanm
Copy link
Contributor Author

seanm commented Feb 15, 2021

I've just confirmed that my warning fixes in this PR are no longer needed vs current develop branch.

So just the H5TEST-dsets remains...

@byrnHDF
Copy link
Contributor

byrnHDF commented Mar 5, 2021

Created #444 issue for dsets test failure

@byrnHDF byrnHDF closed this Mar 5, 2021
vchoi-hdfgroup added a commit that referenced this pull request Apr 22, 2021
vchoi-hdfgroup pushed a commit that referenced this pull request Jan 5, 2022
See Kent's documentation "Designed to Fail Tests and Issues".
(A) Fix for issue #1: HDassert the < and = cases between old and new
entry length.  John will take care of the > case.
(B) Fix for issue #3:  set the cache copy of page_size again if different
    from f->shared->fs_page_size.
@bmribler bmribler mentioned this pull request Jul 2, 2023
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

Successfully merging this pull request may close these issues.

4 participants