Out of bounds read in SiStripMonitorCluster::analyze

[Dr15Jones]

The ASAN report

https://cmssdt.cern.ch/SDT/cgi-bin/logreader/el8_amd64_gcc10/CMSSW_12_5_ASAN_X_2022-07-13-1100/pyRelValMatrixLogs/run/140.58_RunHI2018+RunHI2018+RAWPRIMEHI18+RECOHID18APPROXCLUSTERS+HARVESTDHI18/step3_RunHI2018+RunHI2018+RAWPRIMEHI18+RECOHID18APPROXCLUSTERS+HARVESTDHI18.log#/

Shows a heap-buffer-overflow

[Dr15Jones]

assign dqm

[cmsbuild]

New categories assigned: dqm

@jfernan2,@ahmad3213,@micsucmed,@rvenditti,@emanueleusai,@pmandrik you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

A new Issue was created by @Dr15Jones Chris Jones.

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[Dr15Jones]

The relevant bits of the log are

==5967==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x2af6afd257ff at pc 0x2af63bc921ad bp 0x2af63e0950b0 sp 0x2af63e0950a8
READ of size 1 at 0x2af6afd257ff thread T2
    #0 0x2af63bc921ac in SiStripMonitorCluster::analyze(edm::Event const&, edm::EventSetup const&) (/cvmfs/cms-ib.cern.ch/nweek-02741/el8_amd64_gcc10/cms/cmssw-patch/CMSSW_12_5_ASAN_X_2022-07-13-1100/lib/el8_amd64_gcc10/pluginDQMSiStripMonitorCluster.so+0xa81ac)
    #1 0x2af5e38669bf in edm::stream::EDProducerAdaptorBase::doEvent(edm::EventTransitionInfo const&, edm::ActivityRegistry*, edm::ModuleCallingContext const*) (/cvmfs/cms-ib.cern.ch/nweek-02741/el8_amd64_gcc10/cms/cmssw-patch/CMSSW_12_5_ASAN_X_2022-07-13-1100/lib/el8_amd64_gcc10/libFWCoreFramework.so+0x9109bf)
[cut]

0x2af6afd257ff is located 1 bytes to the left of 10166112-byte region [0x2af6afd25800,0x2af6b06d7760)
allocated by thread T2 here:
    #0 0x2af5e2639607 in operator new(unsigned long) ../../../../libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x2af5f18fe17b in std::vector<unsigned char, std::allocator<unsigned char> >::reserve(unsigned long) (/cvmfs/cms-ib.cern.ch/nweek-02741/el8_amd64_gcc10/cms/cmssw/CMSSW_12_5_ASAN_X_2022-07-11-1100/lib/el8_amd64_gcc10/libDQMServicesCore.so+0x5b17b)
    #2 0x2af60f5553b1 in boost::archive::detail::iserializer<eos::portable_iarchive, std::vector<unsigned char, std::allocator<unsigned char> > >::load_object_data(boost::archive::detail::basic_iarchive&, void*, unsigned int) const (/cvmfs/cms-ib.cern.ch/nweek-02741/el8_amd64_gcc10/cms/cmssw-patch/CMSSW_12_5_ASAN_X_2022-07-13-1100/lib/el8_amd64_gcc10/libCondFormatsCommon.so+0xcd3b1)
    #3 0x2af60f0eff39 in boost::archive::detail::basic_iarchive::load_object(void*, boost::archive::detail::basic_iserializer const&) (/cvmfs/cms-ib.cern.ch/nweek-02741/el8_amd64_gcc10/cms/cmssw-patch/CMSSW_12_5_ASAN_X_2022-07-13-1100/external/el8_amd64_gcc10/lib/libboost_serialization.so.1.78.0+0x17f39)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/cvmfs/cms-ib.cern.ch/nweek-02741/el8_amd64_gcc10/cms/cmssw-patch/CMSSW_12_5_ASAN_X_2022-07-13-1100/lib/el8_amd64_gcc10/pluginDQMSiStripMonitorCluster.so+0xa81ac) in SiStripMonitorCluster::analyze(edm::Event const&, edm::EventSetup const&)
Shadow bytes around the buggy address:
  0x055f55f9caa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x055f55f9cab0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x055f55f9cac0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x055f55f9cad0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x055f55f9cae0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x055f55f9caf0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]
  0x055f55f9cb00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x055f55f9cb10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x055f55f9cb20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x055f55f9cb30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x055f55f9cb40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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
  Shadow gap:              cc
==5967==ABORTING```

[makortel]

FYI @cms-sw/trk-dpg-l2

[Dr15Jones]

Given we are dealing with a read off the end of a std::vector<unsigned char> and that vector comes from boost serialization, it is safe to say it must have come from the PoolDBESSource. Looking at the EventSetup data products used by the module, I'm betting it is SiStripNoises.

The relevant pieces of code are

cmssw/DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc

Line 842 in 2c96f6e

SiStripNoises::Range detNoiseRange = siStripNoises.getRange(detid);

and

cmssw/DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc

Line 872 in 2c96f6e

noise = siStripNoises.getNoise(clusterIter->firstStrip() + iamp, detNoiseRange) /

I bet the call to getRange is given a DetID not in the noise object and the range is then empty. The subsequent call to getNoise will then use the invalid value from range.

[mmusich]

It's worth noting that this shows up in the new RAW' workflow introduced in #38423 by @mandrenguyen.

I bet the call to getRange is given a DetID not in the noise object and the range is then empty

This is not very plausibile. What is more possible is that the strip number passed to get the noise is "wrong" in case of approximate "rectangular" clusters employed in that workflow.

[mmusich]

indeed plugging:

diff --git a/DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc b/DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc
index 8cb59182c9a..b29598f397c 100644
--- a/DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc
+++ b/DQM/SiStripMonitorCluster/src/SiStripMonitorCluster.cc
@@ -868,6 +868,9 @@ void SiStripMonitorCluster::analyze(const edm::Event& iEvent, const edm::EventSe
         for (uint iamp = 0; iamp < ampls.size(); iamp++) {
           if (ampls[iamp] > 0) {  // nonzero amplitude
             cluster_signal += ampls[iamp];
+
+            std::cout << "iamp: " << iamp << " strip number: " << clusterIter->firstStrip() + iamp << std::endl;
+
             if (!siStripQuality.IsStripBad(qualityRange, clusterIter->firstStrip() + iamp)) {
               noise = siStripNoises.getNoise(clusterIter->firstStrip() + iamp, detNoiseRange) /
                       siStripGain.getStripGain(clusterIter->firstStrip() + iamp, detGainRange);

I get before the crash:

iamp: 1 strip number: 765
iamp: 0 strip number: 32767


A fatal system signal has occurred: segmentation violation
The following is the call stack containing the origin of the signal.

the cluster with firstStrip() = 32767 (max uint16_t) doesn't make sense. Probably there's an overflow caused by type mismatches in the reconstruction code.

[mmusich]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

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

[mmusich]

in this line

cmssw/DataFormats/SiStripCluster/src/SiStripCluster.cc

Line 31 in 9e6c192

firstStrip_ = cluster.barycenter() - cluster.width() / 2;

cluster.barycenter() - cluster.width() / 2; can become negative, hence overflowing the uint16_t .

This appears to solve the issue:

diff --git a/DataFormats/SiStripCluster/src/SiStripCluster.cc b/DataFormats/SiStripCluster/src/SiStripCluster.cc
index b03ebb025a9..9a03f666b02 100644
--- a/DataFormats/SiStripCluster/src/SiStripCluster.cc
+++ b/DataFormats/SiStripCluster/src/SiStripCluster.cc
@@ -28,7 +28,7 @@ SiStripCluster::SiStripCluster(const SiStripApproximateCluster cluster) : error_
   amplitudes_.resize(cluster.width(), cluster.avgCharge());
 
   //initialize firstStrip_
-  firstStrip_ = cluster.barycenter() - cluster.width() / 2;
+  firstStrip_ = std::max(0.f, cluster.barycenter() - cluster.width() / 2);
 }