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

EcalMultifitConditionsHostESProducer reads off the end of conditions #44275

Closed
Dr15Jones opened this issue Feb 29, 2024 · 18 comments
Closed

EcalMultifitConditionsHostESProducer reads off the end of conditions #44275

Dr15Jones opened this issue Feb 29, 2024 · 18 comments
Labels
ecal fully-signed heterogeneous-approved reconstruction-approved

Comments

@Dr15Jones
Copy link
Contributor

The ASAN report for CMSSW_14_1_ASAN_X_2024-02-28-2300 has the following item

==1999105==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020020e61f4 at pc 0x1516ecd26b5b bp 0x7ffe98512670 sp 0x7ffe98511e20
READ of size 284 at 0x6020020e61f4 thread T0
    #0 0x1516ecd26b5a in __interceptor_memcpy ../../../../libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
    #1 0x1516b131e593 in alpaka_serial_sync::EcalMultifitConditionsHostESProducer::produce(EcalMultifitConditionsRcd const&) (/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02826/el8_amd64_gcc12/cms/cmssw/CMSSW_14_1_ASAN_X_2024-02-26-2300/lib/el8_amd64_gcc12/pluginRecoLocalCaloEcalRecProducersPluginsPortableSerialSync.so+0xb1593)

...

0x6020020e61f4 is located 0 bytes to the right of 4-byte region [0x6020020e61f0,0x6020020e61f4)
allocated by thread T3 here:
    #0 0x1516ecd976d8 in operator new(unsigned long) ../../../../libsanitizer/asan/asan_new_delete.cpp:95
    #1 0x1516c49e95c8 in std::vector<float, std::allocator<float> >::reserve(unsigned long) (/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02826/el8_amd64_gcc12/cms/cmssw/CMSSW_14_1_ASAN_X_2024-02-26-2300/lib/el8_amd64_gcc12/libDataFormatsTrackerCommon.so+0x175c8)
    #2 0x1516c3ccf54d in boost::archive::detail::iserializer<eos::portable_iarchive, std::vector<float, std::allocator<float> > >::load_object_data(boost::archive::detail::basic_iarchive&, void*, unsigned int) const (/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02826/el8_amd64_gcc12/cms/cmssw/CMSSW_14_1_ASAN_X_2024-02-26-2300/lib/el8_amd64_gcc12/libCondFormatsRunInfo.so+0x15554d)
    #3 0x1516c3fa6c06 in boost::archive::detail::basic_iarchive::load_object(void*, boost::archive::detail::basic_iserializer const&) (/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02826/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_1_ASAN_X_2024-02-28-2300/external/el8_amd64_gcc12/lib/libboost_serialization.so.1.80.0+0x16c06)

Thread T3 created by T0 here:
    #0 0x1516ecd28136 in __interceptor_pthread_create ../../../../libsanitizer/asan/asan_interceptors.cpp:207
    #1 0x1516ea25316f in tbb::detail::r1::rml::internal::thread_monitor::launch(void* (*)(void*), void*, unsigned long) /data/cmsbld/jenkins/workspace/auto-builds/CMSSW_14_0_0_pre2_SKYLAKEAVX512-el8_amd64_gcc12/build/CMSSW_14_0_0_pre2_SKYLAKEAVX512-build/BUILD/el8_amd64_gcc12/external/tbb/v2021.9.0-5109e373398eaf79a8268d6a86a2f6e2/tbb-v2021.9.0/src/tbb/rml_thread_monitor.h:208
    #2 0x1516ea25316f in tbb::detail::r1::rml::private_worker::wake_or_launch() /data/cmsbld/jenkins/workspace/auto-builds/CMSSW_14_0_0_pre2_SKYLAKEAVX512-el8_amd64_gcc12/build/CMSSW_14_0_0_pre2_SKYLAKEAVX512-build/BUILD/el8_amd64_gcc12/external/tbb/v2021.9.0-5109e373398eaf79a8268d6a86a2f6e2/tbb-v2021.9.0/src/tbb/private_server.cpp:305
    #3 0x1516ea25316f in tbb::detail::r1::rml::private_server::wake_some(int) /data/cmsbld/jenkins/workspace/auto-builds/CMSSW_14_0_0_pre2_SKYLAKEAVX512-el8_amd64_gcc12/build/CMSSW_14_0_0_pre2_SKYLAKEAVX512-build/BUILD/el8_amd64_gcc12/external/tbb/v2021.9.0-5109e373398eaf79a8268d6a86a2f6e2/tbb-v2021.9.0/src/tbb/private_server.cpp:412

SUMMARY: AddressSanitizer: heap-buffer-overflow ../../../../libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy
Shadow bytes around the buggy address:
  0x0c0480414be0: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fd
  0x0c0480414bf0: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fa
  0x0c0480414c00: fa fa fd fd fa fa fd fa fa fa fd fd fa fa fd fd
  0x0c0480414c10: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fd
  0x0c0480414c20: fa fa fd fd fa fa 00 00 fa fa 00 00 fa fa fd fa
=>0x0c0480414c30: fa fa 04 fa fa fa 04 fa fa fa 00 00 fa fa[04]fa
  0x0c0480414c40: fa fa fd fd fa fa fd fa fa fa 00 fa fa fa 04 fa
  0x0c0480414c50: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
  0x0c0480414c60: fa fa 00 fa fa fa fd fd fa fa 00 00 fa fa 00 fa
  0x0c0480414c70: fa fa 00 fa fa fa fd fa fa fa fd fa fa fa 00 fa
  0x0c0480414c80: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb

It appears that at least one of the calls to std::memcpy is reading pass the end of the conditions for which it is copying.
@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 29, 2024 •
edited
Loading

cms-bot internal usage

@cmsbuild
Copy link
Contributor

A new Issue was created by @Dr15Jones.

@rappoccio, @smuzaffar, @sextonkennedy, @Dr15Jones, @antoniovilela, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

assign RecoLocalCalo/EcalRecProducers

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction

@jfernan2,@mandrenguyen you have been requested to review this Pull request/Issue and eventually sign? Thanks

@makortel
Copy link
Contributor

assign heterogeneous

@makortel
Copy link
Contributor

FYI @cms-sw/ecal-dpg-l2 @thomreis

@cmsbuild
Copy link
Contributor

New categories assigned: heterogeneous

@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

@fwyzard
Copy link
Contributor

fwyzard commented Feb 29, 2024

type ecal

@cmsbuild cmsbuild added the ecal label Feb 29, 2024
@thomreis thomreis mentioned this issue Mar 4, 2024
Merged
@thomreis
Copy link
Contributor

thomreis commented Mar 4, 2024

Should be fixed with #44301
I will also make a backport PR for 140X.

@thomreis thomreis mentioned this issue Mar 4, 2024
Merged
@thomreis
Copy link
Contributor

thomreis commented Mar 4, 2024

Backport to 140x: #44304

@fwyzard
Copy link
Contributor

fwyzard commented Mar 4, 2024

Just to understand - the fix is just

-                  samplesCorrelationData.EBG6SamplesCorrelation.data(),
+                  samplesCorrelationData.EEG6SamplesCorrelation.data(),

while the others are overall improvements ?

@thomreis
Copy link
Contributor

thomreis commented Mar 4, 2024 •
edited
Loading

Just to understand - the fix is just

-                  samplesCorrelationData.EBG6SamplesCorrelation.data(),
+                  samplesCorrelationData.EEG6SamplesCorrelation.data(),

while the others are overall improvements ?

No that is the typo that got fixed as well.

The real issue was that the std::memcpy did use the maximum possible input size instead of the actual size of the vectors. So more bytes were copied than the input provided.
This was not a problem downstream since those extra bytes were never accessed because the actual size was passed as well, but nevertheless they should not have been copied.

But if the PR changes the output compared to the current version it is likely from the fix of the typo (EB -> EE).

@fwyzard
Copy link
Contributor

fwyzard commented Mar 4, 2024

Understood, thanks.

@antoniovilela
Copy link
Contributor

#44301 is merged.

@makortel
Copy link
Contributor

makortel commented Mar 8, 2024

+heterogeneous

@jfernan2
Copy link
Contributor

+1
Fixed by #44301

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

@makortel
Copy link
Contributor

@cmsbuild, please close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ecal fully-signed heterogeneous-approved reconstruction-approved
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@makortel @Dr15Jones @fwyzard @cmsbuild @jfernan2 @thomreis @antoniovilela