Fix stack-buffer-overflow in EndcapPiZeroDiscriminatorAlgo #21214
Conversation
…Algo::findPreshVector
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @perrotta for master. It involves the following packages: RecoEcal/EgammaClusterAlgos @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
| vector<ESDetId>::iterator itID; | ||
| for (itID = road_2d.begin(); itID != road_2d.end(); itID++) { | ||
| LogTrace("EcalClusters") << "EndcapPiZeroDiscriminatorAlgo: findPreshVectors: ID = " << *itID ; | ||
|
|
||
| float E = 0.; | ||
| RecHitsMap::iterator strip_it = rechits_map->find(*itID); |
There was a problem hiding this comment.
Looking at this my suggestion would also work:
- if(goodPi0Strip(strip_it,last_stripID)) { // continue if strip not found in rechit_map
+ if ((strip_it != rechits_map->end()) && goodPi0Strip(strip_it,last_stripID)) { // continue if strip not found in rechit_map
If rechits_map->find(*itID); returns rechits_map->end() then call to goodPi0Strip would return false. Then you can actually delete some not needed lines. E.g., then you don't need last_stripID. That would overcomplexity here.
There was a problem hiding this comment.
Yes, that also works (and I have nothing against implementing it instead)
The difference is that if rechits_map->find(*itID); returns rechits_map->end(), then with it goodPi0Strip(...) wouldn't even be evaluated, thus not providing the LogTraces which are now in the code.
About the possibility of deleting not needed lines, this goes with my comment in the description of this PR: "I didn't want to modify the logic of the class/method, although I'm sure quite a lot can be done to improve it".
I think this fix addresses the issue reported by you in #21173 with the minimal modification to the code currently in CMSSW. To remove the comparison between the last element of vector road_2d and the first entry of the last element of the RecHitsMap *rechits_map (which I am also convinced as you that it is completely useless, and could be replaced by the very same check that you propose) I think one should first ask the authors of this code why they opted for such a convoluted logic instead, and make sure that there was no hidden particular case to be dealt with with it.
There was a problem hiding this comment.
You are responsible for reconstruction thus it's your call :) As long as it resolves the bug I am happy. Cleaning up the code would be nice-to-have.
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1
|
|
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. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
@davidlange6 could you review this and merge/reject? |
|
merge |
perrotta
deleted the
fixStackBufferOverflowInEndcapPiZeroDiscriminatorAlgo
branch
Fix stack-buffer-overflow, read of 4 bytes, EndcapPiZeroDiscriminatorAlgo::findPreshVector
This fixes the issue reported in #21173
I didn't want to modify the logic of the class/method, although I'm sure quite a lot can be done to improve it.
@davidlt