[RF] RooAddPdf: Avoid UB in checkObservables#9557
[RF] RooAddPdf: Avoid UB in checkObservables#9557guitargeek merged 1 commit intoroot-project:masterfrom
Conversation
A RooAddPdf may have more PDFs than coefficients, in which case "the coefficient of the last PDF is calculated automatically from the condition that the sum of all coefficients has to be 1". In this case, the last call to "_coefList.at(i)" is supposed to return a nullptr because the index is out of range, and dereferencing it is undefined behavior which Clang 13 optimizes away, leading to crashes. Fixes root-project#9547
|
Starting build on |
|
Build failed on mac11/cxx17. Failing tests:
And 4 more |
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Thanks a lot for this fix, great job! 👍
From a technical standpoint, this is the right fix to the issue. However, I'm now concerned what the implications are for the functionality that the overlapping observables check is not done for the last pdf. The last coefficient is implicitly defined as one minus the other one, so possibly it's appropriate to check if any observable of the last pdf is overlapping with one of the other coefficients.
Anyway, it's probably a niche problem that I should take care of in another PR when the more urgent developments for 6.26 are completed.
guitargeek
changed the title
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
|
Build failed on mac1015/python3. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on mac11/cxx17. Errors:
|
|
Build failed on ROOT-ubuntu2004/soversion. Errors:
|
3 participants
A RooAddPdf may have more PDFs than coefficients, in which case "the coefficient of the last PDF is calculated automatically from the condition that the sum of all coefficients has to be 1". In this case, the last call to
_coefList.at(i)is supposed to returna
nullptrbecause the index is out of range, and dereferencing it is undefined behavior which Clang 13 optimizes away, leading to crashes.Fixes #9547