Repository navigation
Conversation
The element count of an STL collection is stored as an Int_t but passed on to TVirtualCollectionProxy::Allocate(UInt_t) or std::vector::resize. When the count comes from corrupt data and is negative, it wraps to billions of elements: in an LHCb DST, a count of 0xa2c35db6 made ROOT allocate and value-initialise 43.7 GB before the reading code could notice anything, getting grid jobs OOM-killed, or throw std::bad_alloc or std::length_error out of TBranch::GetEntry. TBufferFile already rejects such counts for arrays and TBranchElement for split collections, but the collection streaming actions and the collection proxies, including the emulated ones used when reading without a dictionary, did not. Report an error and read the collection as empty instead. The rest of the object is then read from a wrong position until the next byte count check, if any, resynchronises the buffer; it is no worse than reading corrupt data was before. The generic collection streamers used to skip negative sizes silently, either reading the collection as empty or leaving its previous content; they now report them and read the collection as empty too. TConvertClonesArrayToProxy takes the absolute value of the count for backward compatibility, which is undefined for INT_MIN; keep that one negative so that it is rejected too. The check lives in a private header of core/meta, so that the streamers of libCore containers can share it. Assisted-by: claude-code:claude-opus-5-5
A corrupt collection size that is positive but larger than the data can hold is not caught by the negative size check: the collection is resized to up to 2^31 elements, and when reading member-wise the streaming actions then run past the end of the buffer. When each element is known to occupy at least one byte on file, a size larger than the number of bytes left in the buffer is impossible, so report it and read the collection as empty. This holds for collections of basic types, strings and pointers written with an object tag, and for member-wise collections whose value class has, in its layout on file, at least one data member of such a type. It does not hold for objects of class type, which can be written in zero bytes by a custom streamer, including those pointed to by a `//->` member, nor for data members with a custom member streamer, nor for pointers to fundamental types, which collection proxies do not stream at all. Those are left unchecked. The check is restricted to TBufferFile, and its cost is only paid for sizes that exceed the remaining bytes. Bounding by one byte per element rather than by the actual size of an element keeps the check simple; it still lets through sizes up to the size of the buffer, but no longer arbitrary ones. Assisted-by: claude-code:claude-opus-5-5
Strings of more than 254 characters store their length as an Int_t, which ReadStdString and ReadTString passed to std::string::resize and TString::Clobber unchecked. A corrupt length can thus throw std::length_error, or allocate up to 2 GB, before ReadFastArray finds that the buffer does not hold that many characters. ReadCharStar similarly allocated any positive length. Reject negative lengths, and lengths larger than the bytes left in the buffer, and read the string as empty. ReadCharStar keeps ignoring negative lengths silently, as before. Skipping a char* data member that was removed now goes through ReadCharStar too, rather than allocating the buffer itself. This reads the same bytes from a TBufferFile; TBufferJSON and TBufferXML now skip the member the way they wrote it. Assisted-by: claude-code:claude-opus-5-5
TObjArray, TRefArray and TClonesArray read their number of objects as an Int_t and expand their storage to that size before reading the objects. A corrupt count can thus allocate up to 2^31 slots, and for TClonesArray as many objects, before reading them past the end of the buffer. TClonesArray also takes the absolute value of negative counts, which is undefined for INT_MIN and left it negative. Reject negative counts, as for STL collections. Each object of a TObjArray and TRefArray occupies at least 4 bytes on file, and each object of a TClonesArray one byte unless the streamer is bypassed, in which case it depends on the layout of the class. When that holds, a count larger than the bytes left in the buffer can only come from corrupt data, so report it and read the collection as empty too. Assisted-by: claude-code:claude-opus-5-5
The TArray streamers read their size as an Int_t and resize the array to it before ReadFastArray checks that the buffer holds that many elements, so a corrupt size can allocate up to 16 GB for a TArrayD. These arrays hold e.g. the bin contents of histograms. Each element occupies at least one byte on file, so reject sizes that are negative or larger than the bytes left in the buffer, and read the array as empty, as for the other collections. Assisted-by: claude-code:claude-opus-5-5
chrisburr
requested review from
bellenot,
dpiparo,
jblomer and
pcanal
as code owners
October 24, 2026 22:06
1 of 2 tasks
Member
|
I have to review/constrast this in the context of our lifting of the 1GB limit. In first approximation this looks good with the minor concern about the 'expensive' cost in case of collection with content that do not take space (in which case it seems that |
Test Results 22 files 22 suites 3d 11h 3m 39s ⏱️ For more details on these failures, see this check. Results for commit f9dfd35. |
jblomer
reviewed
Oct 27, 2026
Contributor
There was a problem hiding this comment.
I'd like to understand a bit better the context and what we gain by fixing this particular symptom. In particular:
- If the files are compressed, corrupted data should usually be detected by the checksum of the compression algorithm. Is the corruption rate high enough that we see corruptions eluding a 32bit checksum test? Or do the errors occur only in uncompressed blocks?
- Collection sizes make only a fraction of the bytes in the file. If you see problems there, aren't you even more worried about wrong numerical data?
- In case of detected data corruption, I would think we want to abort the process. Here we continue with an error message, leading into an potentially unclear state.
(Side remark: RNTuple data are fully checksummed with a 64bit checksum, so random errors and hardware faults should not go undetected.)
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In LHCb I often see jobs failing with out-of-memory errors due to corrupted inputs or bad reads. These can be quite painful to debug, especially at some grid sites. For the example I studied to produce this MR a corrupt file stored the size of a
std::vectoras0xa2c35db6. ROOT treated this as 2,730,712,502 elements and allocated 43.7 GB insideTBranch::GetEntry.This PR aims to make reading corrupt data fail gracefully instead of allocating huge amounts of memory or reading past the end of the buffer. This appears to be similar to #21814.
The case I was studying is fixed by the first commit (5defd6a) which now fails gracefully with:
I have an additional check a1aaa66 that further ensures that the remaining size of the buffer could plausibly contain the requested data.
Both of these main commits rely on a new private header file
core/meta/res/ROOT/CheckCollectionSize.hxx. This check assumes each element takes at least one byte and its only applied where that is guaranteed.When manually reviewing the changes suggested by Claude I kept finding more issues which are along the same lines. This led to 8cc6bfd, 3bec77b and f9dfd35. I could keep going through finding more cases but I thought it was better to open the PR to get feedback.
Feel free to close this PR if you'd rather solve this a different way or convert this into a bug report instead.
Changes or fixes:
Checklist:
This PR was prepared with the assistance of Claude Code (Opus 5.5).