Skip to content

[io] Additional checks to the properties of the TKey's buffer#21815

Open
dpiparo wants to merge 1 commit intoroot-project:masterfrom
dpiparo:fix_tkey
Open

[io] Additional checks to the properties of the TKey's buffer#21815
dpiparo wants to merge 1 commit intoroot-project:masterfrom
dpiparo:fix_tkey

Conversation

@dpiparo
Copy link
Copy Markdown
Member

@dpiparo dpiparo commented Apr 7, 2026

No description provided.

@dpiparo dpiparo requested review from hahnjo and silverweed April 7, 2026 15:39
@dpiparo dpiparo self-assigned this Apr 7, 2026
@dpiparo dpiparo requested a review from pcanal as a code owner April 7, 2026 15:39
@dpiparo dpiparo added the in:I/O label Apr 7, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 966eafc.

return;
}

if (fNbytes < fKeylen) {
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.

Those 2 numbers are unrelated and this condition is actually valid (fNbytes is the on-file size (compressed size) the data payload and those include the size of the key). If anything, we have an issue if:

  if (fObjlen < fNbytes) {

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.

Isn't the TKey header always uncompressed? If so, this check looks correct to me.

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.

Isn't the TKey header always uncompressed?

yes.

If so, this check looks correct to me.

fKeyLen and fObjLen/fNbytes are still completely unrelated. The total size of the key on disk is fKeyLen + fNbytes. fNbytes can technically even be zero (that's silly but still legal/possible, I think).

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 documentation of fNbytes says: "number of bytes for the compressed object and key." fKeyLen is already included. It must be, because fNbytes is used to navigate along the linked list of keys.

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.

Annoyingly the documentations are inconsistent or unclear. TKey.h says:

   Int_t       fNbytes;      ///< Number of bytes for the object on file

while your quote is from TKey.cxx.
And indeed in addition to the navigation you mention, the code substracts fKeyLen from fNbytes before comparing to fObjLen.

Thanks for clarifying this ! :)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants