ASAN: protection against accessing invalid memory when ieta==0 #31595

smuzaffar · 2020-09-27T11:34:00Z

Workflow 301.0 step3 fails in ASAN IBs [a]. This could happen if ieta==0 (https://github.com/cms-sw/cmssw/blob/master/RecoHI/HiJetAlgos/plugins/ParticleTowerProducer.cc#L181). This PR proposes a protected against ieta==0 and also call fabs once

[a]
https://cmssdt.cern.ch/SDT/cgi-bin/logreader/slc7_amd64_gcc820/CMSSW_11_2_ASAN_X_2020-09-25-2300/pyRelValMatrixLogs/run/301.0_Pyquen_DiJet_pt80to120_2760GeV+Pyquen_DiJet_pt80to120_2760GeV+DIGIHIMIX+RECOHIMIX+HARVESTHI2018PPRECO/step3_Pyquen_DiJet_pt80to120_2760GeV+Pyquen_DiJet_pt80to120_2760GeV+DIGIHIMIX+RECOHIMIX+HARVESTHI2018PPRECO.log#/

0x2afcfb1f93d8 is located 8 bytes to the left of global variable 'etaedge' defined in 'RecoHI/HiJetAlgos/plugins/HITowerHelper.h:5:36'

cmsbuild · 2020-09-27T11:34:28Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-27T11:43:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31595/18650

This PR adds an extra 12KB to repository

cmsbuild · 2020-09-27T11:43:55Z

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

RecoHI/HiJetAlgos

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@yslai, @jazzitup, @yenjie, @kurtejung, @mandrenguyen, @dgulhan, @yetkinyilmaz this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

smuzaffar · 2020-09-27T12:17:08Z

please test

cmsbuild · 2020-09-27T12:17:31Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-26-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9600/console Started: 2020/09/27 14:25

cmsbuild · 2020-09-27T13:36:39Z

+1
Tested at: 9b9b963
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f397e3/9600/summary.html
CMSSW: CMSSW_11_2_X_2020-09-26-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-27T13:36:41Z

Comparison job queued.

cmsbuild · 2020-09-27T16:48:02Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f397e3/9600/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2539438
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2539415
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

perrotta · 2020-09-29T04:12:43Z

ieta=0 can only happen if a tower is filled [1] by one single particle wich has exactly eta=0., whose ieta=0 value is then returned by the function eta2ieta(0.).

It would be interesting to investigate with the debug where such a particle with eta=0. comes out... In any case, it looks to me closer to the spirit of this code assigning to eta the value of the middle pseudorapidity of the innermost tower (i.e. the one with ieta=1) in that case.

This could be achieved by reverting the change proposed here for the ieta2eta function, and rather modify the eta2ieta function above by initializing ieta at line 129 as:

  int ieta = 1;

What do you think @smuzaffar and @mandrenguyen ?

[1]

cmssw/RecoHI/HiJetAlgos/plugins/ParticleTowerProducer.cc

Line 93 in 9b9b963

int ieta = eta2ieta(eta);

smuzaffar · 2020-09-29T06:41:35Z

@perrotta , setting ieta=1 will change the behaviour but I let you and @mandrenguyen decide

mandrenguyen · 2020-09-29T06:55:49Z

Thanks for the fix. I agree with @perrotta

perrotta · 2020-09-29T07:03:23Z

@perrotta , setting ieta=1 will change the behaviour but I let you and @mandrenguyen decide

Not really. If we start by ieta=0 (as it is now) in

cmssw/RecoHI/HiJetAlgos/plugins/ParticleTowerProducer.cc

Line 131 in 9b9b963

while (xeta > hi::etaedge[ieta] && ieta < ietaMax - 1) {

one checks if xeta > hi::etaedge[0] (i.e. xeta > 0.), which is always true except the pathological case xeta==0. which we want to cure with this PR. Then the flow passes to ieta=1 and continues as I suggest.

Therefore, the different behaviour with respect to the present code is only there when xeta==0.:

with the present code in that case there is the crash you are reporting
with wath you propose in that case ieta2eta returns 0.
with what I propose ieta2eta returns the middle pseudorapidity of the innermost tower, as for every other point wich has 0 < xeta < hi::etaedge[1]

cmsbuild · 2020-09-29T07:12:02Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-29T07:15:42Z

The code-checks are being triggered in jenkins.

smuzaffar · 2020-09-29T07:16:46Z

ok, I have update the PR as suggested here #31595 (comment)

cmsbuild · 2020-09-29T07:21:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31595/18668

This PR adds an extra 12KB to repository

cmsbuild · 2020-09-29T07:21:25Z

Pull request #31595 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

perrotta · 2020-09-29T07:36:39Z

please test

cmsbuild · 2020-09-29T07:37:01Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-28-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9635/console Started: 2020/09/29 09:37

cmsbuild · 2020-09-29T08:59:51Z

+1
Tested at: d979bcd
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f397e3/9635/summary.html
CMSSW: CMSSW_11_2_X_2020-09-28-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-29T08:59:54Z

Comparison job queued.

cmsbuild · 2020-09-29T10:31:46Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f397e3/9635/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2539438
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2539409
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

perrotta · 2020-09-29T15:59:36Z

+1

Fixed the pathological case in which there are towers made by particles at eta=0
Jenkins tests pass

cmsbuild · 2020-09-29T16:00:00Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2020-09-29T20:30:25Z

+1

cmsbuild added this to the CMSSW_11_2_X milestone Sep 27, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 27, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 27, 2020

cmsbuild added tests-started and removed tests-pending labels Sep 27, 2020

cmsbuild added tests-approved and removed tests-started labels Sep 27, 2020

cmsbuild added comparison-available and removed comparison-pending labels Sep 27, 2020

cmsbuild removed code-checks-approved comparison-available tests-approved labels Sep 29, 2020

ASAN: protection against accessing invalid memory when ieta==0

d979bcd

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 29, 2020

cmsbuild added tests-started and removed tests-pending labels Sep 29, 2020

cmsbuild added tests-approved and removed tests-started labels Sep 29, 2020

cmsbuild added comparison-available and removed comparison-pending labels Sep 29, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Sep 29, 2020

cmsbuild added orp-approved and removed orp-pending labels Sep 29, 2020

cmsbuild merged commit be09510 into cms-sw:master Sep 29, 2020

mandrenguyen mentioned this pull request Nov 25, 2020

Fix ieta mapping for HF particles CmsHI/cmssw#307

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASAN: protection against accessing invalid memory when ieta==0 #31595

ASAN: protection against accessing invalid memory when ieta==0 #31595

smuzaffar commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

smuzaffar commented Sep 27, 2020

cmsbuild commented Sep 27, 2020 •

edited

cmsbuild commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

perrotta commented Sep 29, 2020

smuzaffar commented Sep 29, 2020

mandrenguyen commented Sep 29, 2020

perrotta commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

smuzaffar commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

perrotta commented Sep 29, 2020

cmsbuild commented Sep 29, 2020 •

edited

cmsbuild commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

perrotta commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

silviodonato commented Sep 29, 2020

ASAN: protection against accessing invalid memory when ieta==0 #31595

ASAN: protection against accessing invalid memory when ieta==0 #31595

Conversation

smuzaffar commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

smuzaffar commented Sep 27, 2020

cmsbuild commented Sep 27, 2020 • edited

cmsbuild commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

cmsbuild commented Sep 27, 2020

perrotta commented Sep 29, 2020

smuzaffar commented Sep 29, 2020

mandrenguyen commented Sep 29, 2020

perrotta commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

smuzaffar commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

perrotta commented Sep 29, 2020

cmsbuild commented Sep 29, 2020 • edited

cmsbuild commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

perrotta commented Sep 29, 2020

cmsbuild commented Sep 29, 2020

silviodonato commented Sep 29, 2020

cmsbuild commented Sep 27, 2020 •

edited

cmsbuild commented Sep 29, 2020 •

edited