[tree] In TBasket/TBranch, error out if reading oob in the streamer#22165
[tree] In TBasket/TBranch, error out if reading oob in the streamer#22165silverweed wants to merge 2 commits into
Conversation
Test Results 22 files 22 suites 3d 9h 23m 44s ⏱️ For more details on these failures, see this check. Results for commit d34cef1. ♻️ This comment has been updated with latest results. |
silverweed
force-pushed
the
tbasket_read_oob
branch
from
bf379ae to
5dd55ea
Compare
silverweed
force-pushed
the
tbasket_read_oob
branch
from
5dd55ea to
d34cef1
Compare
| // the buffer for us. This way we are sure that it will be of the correct size even if the file contains | ||
| // corrupted data. |
There was a problem hiding this comment.
I find that we can do better. With this change we swap the potential problem that the actual number of elements read is larger than the allocated memory (leading to out of bound writes) for the potential problem that the actual number of elements read is smaller than the expected number of elements (leading to out of bounds reads).
Maybe a better way is to explicitly read the size and to use ReadFastArray so that we can do the proper check.
| if (fNevBuf > fNevBufSize) { | ||
| Error( | ||
| "Streamer", | ||
| "Inconsistent length of the entry buffer (%d events for a buffer size of %d). Refusing to deserialize.", |
There was a problem hiding this comment.
| "Inconsistent length of the entry buffer (%d events for a buffer size of %d). Refusing to deserialize.", | |
| "Inconsistent length for the entry offset buffer (%d events for a buffer size of %d). Refusing to deserialize.", |
or
| "Inconsistent length of the entry buffer (%d events for a buffer size of %d). Refusing to deserialize.", | |
| "Entry offset buffer length is smaller than needed (%d events for a buffer size of %d). The basket is not readable.", |
| flag -= 80; | ||
| } | ||
| if (!mustGenerateOffsets && flag && (flag % 10 != 2)) { | ||
| if (fNevBuf > fNevBufSize) { |
There was a problem hiding this comment.
Side note: fNevBuf is the number entries stored in the baskets while fNevBufSize is (in this context) the capacity of the fEntryOffset (and fDisplacement) arrays, so this check is correct.
We may want to add this information explicitly as code comments here.
| b >> n; | ||
| if (n > fMaxBaskets) { | ||
| Error("Streamer", | ||
| "Inconsistent number of baskets: refusing to deserialize. Read %d for an expected maximum of %d.", n, |
There was a problem hiding this comment.
| "Inconsistent number of baskets: refusing to deserialize. Read %d for an expected maximum of %d.", n, | |
| "Inconsistent number of baskets. This basket can not be read. Read %d for the actual number of baskets while we read %d as the value of fMaxBaskets.", n, |
2 participants
TODO
fNevBufSizeand pass nullptr toReadArrayor if we should rather check that they matchChecklist:
This PR fixes #22164