[io] Fix an asan buffer overflow detection in TBufferFile.#22777
[io] Fix an asan buffer overflow detection in TBufferFile.#22777hageboeck 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. |
| // 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]; |
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.
There was a problem hiding this comment.
The alternative to allocating inside ReadArray(Int_t*) is to perform the allocation outside, and pass the already-allocated buffer into ReadArray.
This would amount to (partly) backtracking on fe1be25
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.