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.

Sign up for GitHub

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

Jump to bottom

Fix unitialized memory in H5Tconv.c #4266

Closed
wants to merge 2 commits into from
Closed

Fix unitialized memory in H5Tconv.c #4266

wants to merge 2 commits into from

Conversation

derobins
Copy link
Member

dst_aligned is unitialized and flagged in float --> long double conversions

@derobins
Fix unitialized memory in H5Tconv.c
a1e7dd2 
dst_aligned is unitialized and flagged in float --> long double
conversions
@derobins derobins added Merge - To 1.14 This needs to be merged to HDF5 1.14 Priority - 1. High 🔼 These are important issues that should be resolved in the next release Component - C Library Core C library issues (usually in the src directory) Type - Improvement Improvements that don't add a new feature or functionality labels Mar 27, 2024
@derobins
Copy link
Member Author

derobins commented Mar 27, 2024 •
edited

From the dtransform test:

Testing contiguous, with type conversion (float->ldouble)              PASSED
Uninitialized bytes in __interceptor_pwrite64 at offset 10 inside [0x71f000004608, 3456)
==478000==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7ffff53168d3 in H5FD__sec2_write /home/derobins/hdf5_devel/hdf5/src/H5FDsec2.c:794:27
    #1 0x7ffff51ac982 in H5FD_write /home/derobins/hdf5_devel/hdf5/src/H5FDint.c:317:9
    #2 0x7ffff4ee8813 in H5F__accum_write /home/derobins/hdf5_devel/hdf5/src/H5Faccum.c:821:13
    #3 0x7ffff64a724a in H5PB_write /home/derobins/hdf5_devel/hdf5/src/H5PB.c:991:13
    #4 0x7ffff4fa4166 in H5F_shared_block_write /home/derobins/hdf5_devel/hdf5/src/H5Fio.c:177:9
    #5 0x7ffff4b16525 in H5D__flush_sieve_buf /home/derobins/hdf5_devel/hdf5/src/H5Dint.c:3198:13
    #6 0x7ffff49c2c51 in H5D__contig_flush /home/derobins/hdf5_devel/hdf5/src/H5Dcontig.c:1543:9
    #7 0x7ffff4ae8aca in H5D__flush_real /home/derobins/hdf5_devel/hdf5/src/H5Dint.c:3234:51
    #8 0x7ffff4ae00ac in H5D_close /home/derobins/hdf5_devel/hdf5/src/H5Dint.c:1902:13
    #9 0x7ffff790466b in H5VL__native_dataset_close /home/derobins/hdf5_devel/hdf5/src/H5VLnative_dataset.c:826:9
    #10 0x7ffff779de18 in H5VL__dataset_close /home/derobins/hdf5_devel/hdf5/src/H5VLcallback.c:2770:9
    #11 0x7ffff779c1ec in H5VL_dataset_close /home/derobins/hdf5_devel/hdf5/src/H5VLcallback.c:2807:9
    #12 0x7ffff4b31954 in H5D__close_cb /home/derobins/hdf5_devel/hdf5/src/H5Dint.c:298:9
    #13 0x7ffff5a629ef in H5I__dec_ref /home/derobins/hdf5_devel/hdf5/src/H5Iint.c:973:43
    #14 0x7ffff5a6472f in H5I__dec_app_ref /home/derobins/hdf5_devel/hdf5/src/H5Iint.c:1044:22
    #15 0x7ffff5a66d5f in H5I__dec_app_ref_always_close /home/derobins/hdf5_devel/hdf5/src/H5Iint.c:1153:17
    #16 0x7ffff5a65a2c in H5I_dec_app_ref_always_close /home/derobins/hdf5_devel/hdf5/src/H5Iint.c:1194:22
    #17 0x7ffff47a115c in H5Dclose /home/derobins/hdf5_devel/hdf5/src/H5D.c:480:9
    #18 0x555555654070 in main /home/derobins/hdf5_devel/hdf5/test/dtransform.c:375:5
    #19 0x7ffff3c2814f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #20 0x7ffff3c28208 in __libc_start_main csu/../csu/libc-start.c:360:3
    #21 0x555555573444 in _start (/home/derobins/hdf5_devel/cmake/bin/dtransform+0x1f444) (BuildId: 678491831303bd1ac95987de1b99cca67d0d6263)

  Uninitialized value was stored to memory at
    #0 0x5555555a08fe in __msan_memcpy (/home/derobins/hdf5_devel/cmake/bin/dtransform+0x4c8fe) (BuildId: 678491831303bd1ac95987de1b99cca67d0d6263)
    #1 0x7ffff49e4182 in H5D__contig_writevv_sieve_cb /home/derobins/hdf5_devel/hdf5/src/H5Dcontig.c:1295:13
    #2 0x7ffff79bc879 in H5VM_opvv /home/derobins/hdf5_devel/hdf5/src/H5VM.c:1261:17
    #3 0x7ffff49c0e12 in H5D__contig_writevv /home/derobins/hdf5_devel/hdf5/src/H5Dcontig.c:1500:18
    #4 0x7ffff4bbf3c8 in H5D__scatter_file /home/derobins/hdf5_devel/hdf5/src/H5Dscatgath.c:151:13
    #5 0x7ffff4bb9dab in H5D__scatgath_write /home/derobins/hdf5_devel/hdf5/src/H5Dscatgath.c:783:13
    #6 0x7ffff49bb8d1 in H5D__contig_write /home/derobins/hdf5_devel/hdf5/src/H5Dcontig.c:913:13
    #7 0x7ffff4b63079 in H5D__write /home/derobins/hdf5_devel/hdf5/src/H5Dio.c:825:17
    #8 0x7ffff78f52f4 in H5VL__native_dataset_write /home/derobins/hdf5_devel/hdf5/src/H5VLnative_dataset.c:420:9
    #9 0x7ffff7789b56 in H5VL__dataset_write /home/derobins/hdf5_devel/hdf5/src/H5VLcallback.c:2236:9
    #10 0x7ffff7787c2e in H5VL_dataset_write_direct /home/derobins/hdf5_devel/hdf5/src/H5VLcallback.c:2280:9
    #11 0x7ffff47c1abf in H5D__write_api_common /home/derobins/hdf5_devel/hdf5/src/H5D.c:1315:9
    #12 0x7ffff47bd3b0 in H5Dwrite /home/derobins/hdf5_devel/hdf5/src/H5D.c:1369:9
    #13 0x55555565012b in main /home/derobins/hdf5_devel/hdf5/test/dtransform.c:375:5
    #14 0x7ffff3c2814f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

  Uninitialized value was stored to memory at
    #0 0x5555555a08fe in __msan_memcpy (/home/derobins/hdf5_devel/cmake/bin/dtransform+0x4c8fe) (BuildId: 678491831303bd1ac95987de1b99cca67d0d6263)
    #1 0x7ffff6fe67c4 in H5T__conv_float_ldouble /home/derobins/hdf5_devel/hdf5/src/H5Tconv.c:7375:5
    #2 0x7ffff6ae5143 in H5T_convert_with_ctx /home/derobins/hdf5_devel/hdf5/src/H5T.c:5887:14
    #3 0x7ffff6abe1a9 in H5T_convert /home/derobins/hdf5_devel/hdf5/src/H5T.c:5822:9
    #4 0x7ffff4bb996e in H5D__scatgath_write /home/derobins/hdf5_devel/hdf5/src/H5Dscatgath.c:774:17
    #5 0x7ffff49bb8d1 in H5D__contig_write /home/derobins/hdf5_devel/hdf5/src/H5Dcontig.c:913:13
    #6 0x7ffff4b63079 in H5D__write /home/derobins/hdf5_devel/hdf5/src/H5Dio.c:825:17
    #7 0x7ffff78f52f4 in H5VL__native_dataset_write /home/derobins/hdf5_devel/hdf5/src/H5VLnative_dataset.c:420:9
    #8 0x7ffff7789b56 in H5VL__dataset_write /home/derobins/hdf5_devel/hdf5/src/H5VLcallback.c:2236:9
    #9 0x7ffff7787c2e in H5VL_dataset_write_direct /home/derobins/hdf5_devel/hdf5/src/H5VLcallback.c:2280:9
    #10 0x7ffff47c1abf in H5D__write_api_common /home/derobins/hdf5_devel/hdf5/src/H5D.c:1315:9
    #11 0x7ffff47bd3b0 in H5Dwrite /home/derobins/hdf5_devel/hdf5/src/H5D.c:1369:9
    #12 0x55555565012b in main /home/derobins/hdf5_devel/hdf5/test/dtransform.c:375:5
    #13 0x7ffff3c2814f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

  Uninitialized value was created by an allocation of 'dst_aligned' in the stack frame
    #0 0x7ffff6fe1570 in H5T__conv_float_ldouble /home/derobins/hdf5_devel/hdf5/src/H5Tconv.c:7375:5

@jhendersonHDF
Copy link
Collaborator

Strange. Do we know if this is from uninitialized data coming from the test?

@derobins
Copy link
Member Author

derobins commented Mar 27, 2024 •
edited

Strange. Do we know if this is from uninitialized data coming from the test?

It's a stack-allocated variable, so no. It's also being used as a destination, not a source.

DT      dst_aligned;        /*destination aligned type    */

@jhendersonHDF
Copy link
Collaborator

Strange. Do we know if this is from uninitialized data coming from the test?

It's a stack-allocated variable, so no. It's also being used as a destination, not a source.

DT      dst_aligned;        /*destination aligned type    */

Uninitialized value was created by a heap allocation
#0 0x55f21a12aa12 in __interceptor_malloc /tmp/llvm-build-16.0.6/llvm-16.0.6.src/projects/compiler-rt/lib/msan/msan_interceptors.cpp:934:3
#1 0x7fccb6a79b4e in H5D__typeinfo_init_phase3 /home/jhenderson/git/hdf5/src/H5Dio.c:1451:51

It's coming from a type conversion buffer that's malloced in the library:

https://github.com/HDFGroup/hdf5/blob/develop/src/H5Dio.c#L1451

@jhendersonHDF
Copy link
Collaborator

In 93754ca, I see "* Rework type conversion buffer allocation. Only one buffer is shared
between datasets in mdset mode, and it is malloced instead of calloced.". But, it looks like we may be using elements out of the type conversion buffer that aren't initialized since it's malloced.

@derobins
Copy link
Member Author

Uninitialized value was created by an allocation of 'dst_aligned' in the stack frame
#0 0x7ffff6fe1570 in H5T__conv_float_ldouble /home/derobins/hdf5_devel/hdf5/src/H5Tconv.c:7375:5

But the last line of the sanitizer output:

Uninitialized value was created by an allocation of 'dst_aligned' in the stack frame
    #0 0x7ffff6fe1570 in H5T__conv_float_ldouble /home/derobins/hdf5_devel/hdf5/src/H5Tconv.c:7375:5

@jhendersonHDF
Copy link
Collaborator

Uninitialized value was created by an allocation of 'dst_aligned' in the stack frame
#0 0x7ffff6fe1570 in H5T__conv_float_ldouble /home/derobins/hdf5_devel/hdf5/src/H5Tconv.c:7375:5

But the last line of the sanitizer output:

Uninitialized value was created by an allocation of 'dst_aligned' in the stack frame
    #0 0x7ffff6fe1570 in H5T__conv_float_ldouble /home/derobins/hdf5_devel/hdf5/src/H5Tconv.c:7375:5

I'm guessing we get different results based on whether free lists are enabled or not. With them disabled, I can see that dst_aligned gets an uninitialized value because it was stored into from a malloced buffer that wasn't initialized for the element that was stored in it. callocing that buffer clears the warning too and doesn't need a memset in the type conversion code.

@jhendersonHDF
Copy link
Collaborator

That said, it seems like the buffer was switched from calloc to malloc for a reason (probably performance). But it'd also be nice to solve the problem where it originates from rather than in the type conversion code, if possible.

@derobins derobins marked this pull request as draft March 28, 2024 16:20
@qkoziol
Copy link
Contributor

qkoziol commented Mar 28, 2024

I have a feeling that it's not in the type conversion macro also (i.e. the memset is masking the root cause)

@bmribler bmribler self-requested a review April 2, 2024 02:50
@derobins derobins mentioned this pull request May 28, 2024
Open
@derobins
Copy link
Member Author

Closing since we'll fix this closer to the root cause. See issue #4529.

@derobins derobins closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyoklee hyoklee hyoklee approved these changes

@lrknox lrknox Awaiting requested review from lrknox lrknox is a code owner

@byrnHDF byrnHDF Awaiting requested review from byrnHDF byrnHDF is a code owner

@fortnern fortnern Awaiting requested review from fortnern fortnern is a code owner

@jhendersonHDF jhendersonHDF Awaiting requested review from jhendersonHDF jhendersonHDF is a code owner

@qkoziol qkoziol Awaiting requested review from qkoziol qkoziol is a code owner

@vchoi-hdfgroup vchoi-hdfgroup Awaiting requested review from vchoi-hdfgroup vchoi-hdfgroup is a code owner

@glennsong09 glennsong09 Awaiting requested review from glennsong09 glennsong09 is a code owner

@mattjala mattjala Awaiting requested review from mattjala mattjala is a code owner

@brtnfld brtnfld Awaiting requested review from brtnfld brtnfld is a code owner

@bmribler bmribler Awaiting requested review from bmribler bmribler is a code owner

Assignees
No one assigned
Labels
Component - C Library Core C library issues (usually in the src directory) Merge - To 1.14 This needs to be merged to HDF5 1.14 Priority - 1. High 🔼 These are important issues that should be resolved in the next release Type - Improvement Improvements that don't add a new feature or functionality
Projects
Downstream HDF5 Merges
Status: Merges Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@derobins @jhendersonHDF @qkoziol @hyoklee @bmribler