I/O regression since 6.36: member-wise reading/writing of a variable-size array in a base class uses the wrong counter offset

[tamasgal]

Check duplicate issues.

• Checked for duplicates

Description

We are using ROOT as the I/O format of the KM3NeT experiment and encountered an issue when evaluating the upgrade of ROOT to 6.36: PMT hit data cannot be read anymore and results in segfaults in our main C++/ROOT framework, which is at this moment still closed source.

Reading the files with my own pure-Julia ROOT I/O library KM3io.jl based on UnROOT.jl works fine and was helpful with my investigations below.

I think the issue is that since 6.36.00, a variable-size array of objects (T* p; //[n], i.e. a TStreamerLoop
element) is read and written with the wrong counter offset when

1. the array lives in a base class that sits at a non-zero offset in the value type, and
2. the value type is stored in a collection that is streamed member-wise.

The loop counter is taken from an address that is not shifted by the base-class offset, so
the loop runs the wrong number of iterations. On read the array pointer stays NULL (or is
filled with garbage); on write the payload is silently dropprd. 6.32 and the whole 6.34
series are fine (well I did not test each of them but went through the relevant part of the code and it's unchanged in 6.38.00, 6.40.02 and current master (76f8fd3f1a from yesterday night ;).

This is what's so serious in KM3NeT: our raw data (KM3NETDAQ::JDAQTimeslice, a
std::vector<JDAQSuperFrame> where each JDAQSuperFrame has a JDAQFrame base holding
JDAQHit* buffer; //[numberOfHits]) can no longer be read with ROOT >= 6.36. The
user-visible symptom is

Error in <TBufferFile::CheckByteCount>: object of class buffer read too few bytes: 2 instead of 254

followed by a segmentation fault when the (non-empty but NULL) buffer is dereferenced.

Versions

• 6.32.04 OK
• 6.34.00, 6.34.10 | OK (source-identical action code)
• 6.36.00 broken
• 6.38.00 broken
• 6.40.02, current master (76f8fd3) broken

Reproduced in the official rootproject/root Docker images (linux/amd64) on my macOS.
I put a minimal reproducer below(!).

Here is a summary of my ROOT source analysis:

All line numbers in io/io/src/TStreamerInfoActions.cxx are identical in v6-36-00 and
master (76f8fd3). Member-wise reading of a kBase element builds an offset-shifted sub-sequence:

// GetCollectionReadAction, case TStreamerInfo::kBase  (line 3716; write side line 4035)
baseActions->AddToOffset(baseEl->GetOffset());

So every action inside the base sub-sequence is expected to carry the base offset in its own
fOffset. TConfStreamerLoop (line 239) does not participate:

struct TConfStreamerLoop : public TConfiguration {
   bool fIsPtrPtr = false;
   TConfStreamerLoop(TVirtualStreamerInfo *info, UInt_t id, TCompInfo_t *compinfo, Int_t offset, bool isPtrPtr)
      : TConfiguration(info, id, compinfo, offset), fIsPtrPtr(isPtrPtr) {}
   TConfiguration *Copy() override { return new TConfStreamerLoop(*this); };
};

It inherits the plain TConfiguration::AddToOffset (which shifts only fOffset), and the
five loop bodies read the counter from config->fCompInfo->fMethod, which is the counter
offset within the declaring class and is never rebased:

// lines 1575, 1589, 1632, 1726, 1824, e.g.
Int_t vlen = *((Int_t*) ((char *) addr + config->fCompInfo->fMethod /*counter offset*/));
char **pp  = (char **)((char *)addr + config->fOffset);   // data pointer: correctly rebased

fCompInfo is owned by the TStreamerInfo and shared, so AddToOffset cannot fix it in
place. The data pointer (fOffset) is rebased by the base offset, the counter is not, and
the two disagree by exactly the base-class offset.

In our real case JDAQFrame is the third base of JDAQSuperFrame at offset 88, and its
counter is read from superframe + 8 (which is TObject::fUniqueID, 0 in our data) instead
of superframe + 96. Hence 0 iterations, NULL buffer, and the CheckByteCount message.
Note this is also a memory-safety issue: the counter is whatever happens to live at the
wrong offset, so a non-zero value there yields an out-of-bounds read into the shared
member-wise block rather than a clean NULL.

Before 6.36 there was no kStreamLoop case in the collection action factory; it fell
through to Looper::GenericRead, which passed the base offset as eoffset to
TStreamerInfo::ReadBuffer, where the counter read is eoffset + fMethod. So the offset
arrived correctly.

A FiX?

I am not a ROOT/C++ expert by all means but a fix could be to give
give TConfStreamerLoop its own absolute counter offset and keep it in sync in
AddToOffset, then read the counter from it at the mentioned li9nes...

struct TConfStreamerLoop : public TConfiguration {
   bool  fIsPtrPtr = false;
   Int_t fCounterOffset = 0;   // absolute, relative to the collection element
   TConfStreamerLoop(TVirtualStreamerInfo *info, UInt_t id, TCompInfo_t *compinfo, Int_t offset, bool isPtrPtr)
      : TConfiguration(info, id, compinfo, offset), fIsPtrPtr(isPtrPtr),
        fCounterOffset((Int_t)compinfo->fMethod) {}
   void AddToOffset(Int_t delta) override {
      if (fOffset != TVirtualStreamerInfo::kMissing) { fOffset += delta; fCounterOffset += delta; }
   }
   TConfiguration *Copy() override { return new TConfStreamerLoop(*this); }
};

i.e. replace config->fCompInfo->fMethod with the per-configuration fCounterOffset. For
the scalar (non-collection) path AddToOffset is never called, so fCounterOffset stays
equal to fMethod and behaviour is unchanged.

Happy to test a patch against our files and the reproducer above :)

Reproducer

Four small classes. Frame owns the //[n] array; Super puts Frame behind another
base (Pad) so Frame is at a non-zero offset; the branch is a std::vector<Super>,
which ROOT streams member-wise by default.

repro.h:

#ifndef REPRO_H
#define REPRO_H

#include <vector>
#include <Rtypes.h>

// A small object stored in a variable-size array (-> TStreamerLoop / kStreamLoop)
class Hit {
public:
   Hit() : a(0), b(0) {}
   Hit(int x, int y) : a(x), b(y) {}
   int a;
   int b;
   ClassDefNV(Hit, 1)
};

// Holds the leaf-count array hits with counter n
class Frame {
public:
   Frame() : n(0), hits(nullptr) {}
   ~Frame() { delete[] hits; }
   Frame(const Frame &f) : n(0), hits(nullptr) { set(f.n, f.hits); }
   Frame &operator=(const Frame &f) { if (this != &f) set(f.n, f.hits); return *this; }
   void set(int m, const Hit *d)
   {
      delete[] hits; hits = nullptr; n = 0;
      if (m > 0) { hits = new Hit[m]; for (int i = 0; i < m; ++i) hits[i] = d[i]; n = m; }
   }
   int  n;    //
   Hit *hits; //[n]
   ClassDef(Frame, 1)
};

// A base that precedes Frame, so that the Frame base does NOT sit at ofset 0
class Pad {
public:
   Pad() : x(0) {}
   int x;
   ClassDef(Pad, 1)
};

// The cpllection element. Frame is a base at a non-zero offset.
class Super : public Pad, public Frame {
public:
   Super() {}
   ClassDef(Super, 1)
};

#endif

LinkDef.h:

#ifdef __ROOTCLING__
#pragma link off all globals;
#pragma link off all classes;
#pragma link off all functions;

#pragma link C++ class Hit + ;
#pragma link C++ class Frame + ;
#pragma link C++ class Pad + ;
#pragma link C++ class Super + ;
#pragma link C++ class std::vector < Super> + ;
#endif

main.cc:

#include "repro.h"
#include <TFile.h>
#include <TTree.h>
#include <TROOT.h>
#include <vector>
#include <cstdio>
#include <cstring>

static std::vector<Super> makeSlice()
{
   std::vector<Super> v(4);
   for (int f = 0; f < 4; ++f) {
      int m = f + 1;                       // just a dumb 1,2,3,4 hits per frame
      std::vector<Hit> h(m);
      for (int i = 0; i < m; ++i) h[i] = Hit(100 * f + i, 7);
      v[f].set(m, h.data());
      v[f].x = 0;
   }
   return v;
}

static void write(const char *fn)
{
   TFile f(fn, "RECREATE");
   TTree t("T", "T");
   std::vector<Super> *slice = new std::vector<Super>(makeSlice());
   t.Branch("slice", &slice);  // default split level
   t.Fill();
   t.Write();
   f.Close();
   printf("ROOT %s : wrote %s\n", gROOT->GetVersion(), fn);
}

static int read(const char *fn)
{
   int expected = 1 + 2 + 3 + 4;
   int got = 0, nullbuf = 0;
   TFile f(fn);
   TTree *t = (TTree *)f.Get("T");
   std::vector<Super> *slice = nullptr;
   t->SetBranchAddress("slice", &slice);
   t->GetEntry(0);
   for (size_t i = 0; i < slice->size(); ++i) {
      const Frame &fr = (*slice)[i];
      if (fr.n > 0 && fr.hits == nullptr) ++nullbuf;
      got += (fr.hits ? fr.n : 0);
   }
   f.Close();
   printf("ROOT %s : expected hits=%d  read hits=%d  frames_with_n>0_but_null_buffer=%d  -> %s\n",
          gROOT->GetVersion(), expected, got, nullbuf, (got == expected && nullbuf == 0) ? "PASS" : "FAIL");
   return (got == expected && nullbuf == 0) ? 0 : 1;
}

int main(int argc, char **argv)
{
   const char *fn = argc > 2 ? argv[2] : "repro.root";
   if (argc > 1 && strcmp(argv[1], "write") == 0) { write(fn); return 0; }
   if (argc > 1 && strcmp(argv[1], "read") == 0)  return read(fn);
   write(fn);
   return read(fn);
}

Build and run:

rootcling -f dict.cxx repro.h LinkDef.h
g++ -std=c++17 -O0 -o repro main.cc dict.cxx $(root-config --cflags --libs)
./repro

Single process, write then read:

# ROOT 6.32.04
ROOT 6.32.04 : wrote repro.root
ROOT 6.32.04 : expected hits=10  read hits=10  frames_with_n>0_but_null_buffer=0  -> PASS

# ROOT 6.36.00 (and 6.38.00)
ROOT 6.36.000 : wrote repro.root
ROOT 6.36.000 : expected hits=10  read hits=0  frames_with_n>0_but_null_buffer=4  -> FAIL

On 6.36 the write side already drops the hits, so read-back is silently empty and no error
is printed. This is the more worrying half: a 6.36 writer produces hit-free files that a
6.36 reader accepts without complaint.

Writing a correct file with 6.32 and reading it with 6.36 gives the exact user-facing error
and the NULL buffers:

# with ROOT 6.32
./repro write good.root
# with ROOT 6.36
./repro read good.root

Error in <TBufferFile::CheckByteCount>: object of class hits read too few bytes: 2 instead of 142
ROOT 6.36.000 : expected hits=10  read hits=0  frames_with_n>0_but_null_buffer=4  -> FAIL

142 = 2 (version) + 10 hits * 14 bytes (the member-wise hits block for the whole
collection has one byte count; the loop consumes 0 hits, i.e. only the 2 version bytes).

ROOT version

6.36+

Installation method

build from source

Operating system

Linux, macOS

Additional context

No response

[tamasgal]

I prepared a fix with regression tests in #22901