Skip to content

[io] Add buffer bound checks in TFile::Recover#22190

Open
silverweed wants to merge 5 commits into
root-project:masterfrom
silverweed:tfile_init_oob
Open

[io] Add buffer bound checks in TFile::Recover#22190
silverweed wants to merge 5 commits into
root-project:masterfrom
silverweed:tfile_init_oob

Conversation

@silverweed
Copy link
Copy Markdown
Contributor

@silverweed silverweed commented May 8, 2026

TODO

  • add tests
  • decide if we want/need to use the safer overloads elsewhere

This PR fixes #22169

@silverweed silverweed requested a review from jblomer May 8, 2026 13:10
@silverweed silverweed self-assigned this May 8, 2026
@silverweed silverweed force-pushed the tfile_init_oob branch 4 times, most recently from 668624f to 33f969c Compare May 8, 2026 13:46
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

Test Results

    22 files      22 suites   3d 11h 34m 26s ⏱️
 3 852 tests  3 851 ✅ 0 💤 1 ❌
76 069 runs  76 068 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 986cb8f.

♻️ This comment has been updated with latest results.

@silverweed silverweed marked this pull request as ready for review May 11, 2026 09:31
Comment thread io/io/src/TKey.cxx
fRemainingBufSize -= additionalBytesNeeded;
return true;
}
} RequireAdditionalBufCapacity{this, bufsize};
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.

The name invoke the converse of the usage. It sounds like we are reserving/add capacity to be written to where as the actual operation is closer to Consuming some of the existing capacity (with no possibility to add to it (i.e. no additional)). What about something like AdvanceCursor or ConsumeCapacity or ReserveSpaceInOutputBuffer ?

Copy link
Copy Markdown
Contributor Author

@silverweed silverweed May 19, 2026

Choose a reason for hiding this comment

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

I think the verb is appropriate, as we require that many additional bytes in order to proceed: if our requirement isn't met, we bail out and fail.

Could maybe be called Ensure instead of Require, as that's a sort-of-convention we do use from time to time

This prevents potential oob stack reads in case of corrupted TFiles
- Declare variables when they're used
- Spare a needless dynamic allocation
- Use memcpy instead of frombuf in a loop
@silverweed silverweed added this to the 6.40.02 milestone May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[io] TFile::Recover doesn't properly validate read lengths

3 participants