[io] Fix an asan buffer overflow detection in TBufferFile.#22777
Open
hageboeck wants to merge 1 commit into
Open
[io] Fix an asan buffer overflow detection in TBufferFile.#22777hageboeck wants to merge 1 commit into
hageboeck wants to merge 1 commit into
Conversation
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.
Test Results 23 files 23 suites 3d 15h 37m 16s ⏱️ For more details on these failures, see this check. Results for commit 9c994b2. ♻️ This comment has been updated with latest results. |
jblomer
approved these changes
Jul 9, 2026
pcanal
reviewed
Jul 9, 2026
| // 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]; |
Member
There was a problem hiding this comment.
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.
pcanal
requested changes
Jul 9, 2026
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.
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
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.