Skip to content

Commit 133c2ec

Browse files
committed
CalDet<TPCPadFlags>: Protect against invalid reads
A continuation of the CalDet<TPCFlags> saga, possibly related to https://its.cern.ch/jira/browse/O2-4671 Tests on ARM, even after deployment of the custom streamer in #14830, still showed segfaults in TPC digitization. With the relevant code isolated into a unit test in #14850, it was possible to do a valgrind study. This showed Invalid reads to the mData of CalArray. Thereafter, putting assert statements showed that we often access CalArray<PadFlags> data slightly out of bounds - irrespective of custom streamer or not. This then either indicates a problem in the code logic or a problem with the calibration CCDB objects. This should clearly be fixed. In the meantime, this commit adds a protection against invalid accesses and returns a trivial answer as well as an error message. This is in any case better than undefined behaviour. In addition, this commit introduces possibility to switch off the custom streamer for further studies.
1 parent 0fb4692 commit 133c2ec

File tree

3 files changed

+39
-3
lines changed

3 files changed

+39
-3
lines changed

Detectors/TPC/base/include/TPCBase/CalArray.h

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@
2626
#include <boost/format.hpp>
2727
#endif
2828

29+
#ifdef NDEBUG
30+
#undef NDEBUG
31+
#include <cassert>
32+
#endif
33+
2934
namespace o2
3035
{
3136
namespace tpc
@@ -93,7 +98,11 @@ class CalArray
9398
int getPadSubsetNumber() const { return mPadSubsetNumber; }
9499

95100
void setValue(const size_t channel, const T& value) { mData[channel] = value; }
96-
const T getValue(const size_t channel) const { return mData[channel]; }
101+
const T getValue(const size_t channel) const
102+
{
103+
assert(channel < mData.size());
104+
return mData[channel];
105+
}
97106

98107
void setValue(const size_t row, const size_t pad, const T& value);
99108
const T getValue(const size_t row, const size_t pad) const;

Detectors/TPC/base/include/TPCBase/CalDet.h

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,12 @@
3030
#include "Rtypes.h"
3131
#endif
3232

33+
#ifndef NDEBUG
34+
#undef NDEBUG
35+
// always enable assert
36+
#include <cassert>
37+
#endif
38+
3339
namespace o2
3440
{
3541
namespace tpc
@@ -211,7 +217,26 @@ inline const T CalDet<T>::getValue(const ROC roc, const size_t row, const size_t
211217
}
212218
case PadSubset::Region: {
213219
const auto globalRow = roc.isOROC() ? mappedRow + mapper.getNumberOfRowsROC(ROC(0)) : mappedRow;
214-
return mData[Mapper::REGION[globalRow] + roc.getSector() * Mapper::NREGIONS].getValue(Mapper::OFFSETCRUGLOBAL[globalRow] + mappedPad);
220+
const auto dataRow = Mapper::REGION[globalRow] + roc.getSector() * Mapper::NREGIONS;
221+
const auto index = Mapper::OFFSETCRUGLOBAL[globalRow] + mappedPad;
222+
assert(dataRow < mData.size());
223+
if (index >= mData[dataRow].getData().size()) {
224+
// S. Wenzel: We shouldn't come here but we do. For instance for CalDet calibrations loaded from
225+
// creator.loadIDCPadFlags(1731274461770);
226+
227+
// In this case there is an index overflow, leading to invalid reads and potentially a segfault.
228+
// To increase stability, for now returning a trivial answer. This can be removed once either the algorithm
229+
// or the calibration data has been fixed.
230+
#ifndef GPUCA_ALIGPUCODE // hide from GPU standalone compilation
231+
static bool printMsg = true;
232+
if (printMsg) {
233+
LOG(error) << "Out of bound access in TPC CalDet ROC " << roc << " row " << row << " pad " << pad << " (no more messages printed)";
234+
}
235+
printMsg = false;
236+
#endif
237+
return T{};
238+
}
239+
return mData[dataRow].getValue(index);
215240
break;
216241
}
217242
}

Detectors/TPC/base/src/TPCFlagsMemberCustomStreamer.cxx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,9 @@ static __attribute__((used)) int _R__dummyStreamer_3 =
6969
([]() {
7070
auto cl = TClass::GetClass<o2::tpc::CalArray<o2::tpc::PadFlags>>();
7171
if (cl) {
72-
cl->AdoptMemberStreamer("mData", new TMemberStreamer(MemberVectorPadFlagsStreamer));
72+
if (!getenv("TPC_PADFLAGS_STREAMER_OFF")) {
73+
cl->AdoptMemberStreamer("mData", new TMemberStreamer(MemberVectorPadFlagsStreamer));
74+
}
7375
} else {
7476
// we should never come here ... and if we do we should assert/fail
7577
assert(false);

0 commit comments

Comments
 (0)