Skip to content

[io] Fix an asan buffer overflow detection in TBufferFile.#22777

Open
hageboeck wants to merge 1 commit into
root-project:masterfrom
hageboeck:asan_TBufferFile
Open

[io] Fix an asan buffer overflow detection in TBufferFile.#22777
hageboeck wants to merge 1 commit into
root-project:masterfrom
hageboeck:asan_TBufferFile

Conversation

@hageboeck

@hageboeck hageboeck commented Jul 8, 2026

Copy link
Copy Markdown
Member

As a followup to fe1be25, increase the auto-allocated array buffer by one element. In fe1be25 (PR #22165), the external allocation in TBasket (size == fNevBufSize) was replaced with an automatic allocation in TBufferFile::ReadArray(Int_t), whose size was the number of elements read from the file.
However, when ReadArray(Int_t) is used as an offset array, it is assumed to contain one more element than visible from the size field stored on disc. See here:

root/tree/tree/src/TBasket.cxx

Lines 1250 to 1279 in da08f46

if (entryOffset) {
bool hasOffsetBit = fIOBits & static_cast<UChar_t>(TBasket::EIOBits::kGenerateOffsetMap);
if (!CanGenerateOffsetArray()) {
// If we have set the offset map flag, but cannot dynamically generate the map, then
// we should at least convert the offset array to a size array. Note that we always
// write out (fNevBuf+1) entries to match the original case.
if (hasOffsetBit) {
for (Int_t idx = fNevBuf; idx > 0; idx--) {
entryOffset[idx] -= entryOffset[idx - 1];
}
entryOffset[0] = 0;
}
fBufferRef->WriteArray(entryOffset, fNevBuf + 1);
// Convert back to offset format: keeping both sizes and offsets in-memory were considered,
// but it seems better to use CPU than memory.
if (hasOffsetBit) {
entryOffset[0] = fKeylen;
for (Int_t idx = 1; idx < fNevBuf + 1; idx++) {
entryOffset[idx] += entryOffset[idx - 1];
}
}
} else if (!hasOffsetBit) { // In this case, write out as normal
fBufferRef->WriteArray(entryOffset, fNevBuf + 1);
}
if (fDisplacement) {
fBufferRef->WriteArray(fDisplacement, fNevBuf + 1);
delete[] fDisplacement;
fDisplacement = nullptr;
}
}

The auto-allocated buffer needs to be large enough to cover this case, so here we propose to generally over-allocate the buffer by one int.

To give some numbers from this particular case:
Before #22165, the offset array buffer was sized using fNevBufSize, 1000 in this case.
After the PR, it was 84, but we would have needed 85 elements to fit it.
What we propose is to overallocate all buffer for ReadArray(Int_t), leading to generally smaller buffers for offset arrays, and to one additional int for arrays that are not used as offset arrays.

Thanks to @silverweed for helping to investigate.

As a followup to fe1be25, increase the auto-allocated array buffer by
one element. In fe1be25 (PR root-project#22165), the external allocation in TBasket
(size == fNevBufSize) was replaced with an automatic allocation in
TBufferFile::ReadArray(Int_t), whose size was the number of elements
read from the file.
However, when the integer array is used as an offset array, it is
assumed to contain one more element. The auto-allocated buffer needs to
be large enough to cover this case.
@hageboeck hageboeck self-assigned this Jul 8, 2026
@hageboeck hageboeck requested a review from silverweed July 8, 2026 16:04
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 15h 37m 16s ⏱️
 3 872 tests  3 866 ✅   0 💤 6 ❌
79 695 runs  79 584 ✅ 105 💤 6 ❌

For more details on these failures, see this check.

Results for commit 9c994b2.

♻️ This comment has been updated with latest results.

@hageboeck hageboeck marked this pull request as ready for review July 9, 2026 09:55
@hageboeck hageboeck requested review from jblomer and pcanal as code owners July 9, 2026 09:55
Comment thread io/io/src/TBufferFile.cxx
// Allocate a buffer for this array if the caller hasn't done so.
// Note: Offset arrays require one more element than n claims, so we need to overallocate.
if (!ii)
ii = new Int_t[n + 1];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to be a fixed need to one particular case of the use of ReadArray that is costing (a bit) every other use of ReadArray (apriori there are many distinct ones). I/we would have to look at the particular case (offset array) and see if there isn't any better (aka better localized) solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants