cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.09k stars 4.32k forks source link

[UBSAN] TRandom3: out of bound read runtime error #46437

Open smuzaffar opened 1 month ago

smuzaffar commented 1 month ago

For UBSAN relvals, we do see runtime errors like [a]. This was fixed for root 6.32 and above. Is this a change which we should get in root 6.30 used by CMSSW_14_0 and above?

[a] https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/raw/el8_amd64_gcc12/CMSSW_14_2_UBSAN_X_2024-10-16-1100/pyRelValMatrixLogs/run/11634.914_TTbar_14TeV+2021_DDDDB/step1_TTbar_14TeV+2021_DDDDB.log

root/6.30.09-c59d6b036f1cbd6988c172ba319259f1/include/TRandom3.h:38:62: runtime error: index 624 out of bounds for type 'unsigned int [624]'
    #0 0x1548b96a4fcf in TRandom3::GetSeed() const root/6.30.09-c59d6b036f1cbd6988c172ba319259f1/include/TRandom3.h:38
    #1 0x1548b96a4fcf in edm::TRandomAdaptor::TRandomAdaptor(long) src/IOMC/RandomEngine/src/TRandomAdaptor.cc:18
smuzaffar commented 1 month ago

assign core

cmsbuild commented 1 month ago

New categories assigned: core

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

cmsbuild commented 1 month ago

cms-bot internal usage

cmsbuild commented 1 month ago

A new Issue was created by @smuzaffar.

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

cms-bot commands are listed here

makortel commented 1 month ago

In principle I'd be in favor of backporting the fix, however given the discussion in https://github.com/root-project/root/pull/14135 the backport would likely change behavior. But I don't understand the behavior of https://github.com/cms-sw/cmssw/blob/master/IOMC/RandomEngine/src/TRandomAdaptor.cc and CLHEP::HepRandomEngine well enough to predict it it really would change (the return value of TRandom3::GetSeed() is stored in theSeed member variable, which is not used in TRandomAdaptor, but maybe it is used in the CLHEP::HepRandomEngine base class; but the discussion in https://github.com/root-project/root/pull/14135 and comments in ROOT code indicate that the TRandom3::GetSeed() return value is mostly useless anyway).

I'm adding @cms-sw/simulation-l2 since they are the main users of random numbers and might have some insight.

The TRandom3 is perhaps used only in https://github.com/cms-sw/cmssw/blob/eee03e98a405ded01885342963342157c198d10d/IOMC/RandomEngine/python/IOMC_cff.py#L20-L23

smuzaffar commented 23 hours ago

@cms-sw/simulation-l2 , any thoughts?

smuzaffar commented 23 hours ago

assign simulation

cmsbuild commented 23 hours ago

New categories assigned: simulation

@civanch,@kpedro88,@mdhildreth you have been requested to review this Pull request/Issue and eventually sign? Thanks