Skip to content

Fix Global Heap over-read on sub-header trailing padding - #261

Merged
bnlawrence merged 2 commits into
NCAS-CMS:mainfrom
ajelenak:fix-global-heap-over-read
Sep 1, 2026
Merged

Fix Global Heap over-read on sub-header trailing padding#261
bnlawrence merged 2 commits into
NCAS-CMS:mainfrom
ajelenak:fix-global-heap-over-read

Conversation

@ajelenak

@ajelenak ajelenak commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Description

GlobalHeap.objects looped while offset < len(self.heap_data), so it entered the loop with fewer than a full 16-byte GLOBAL_HEAP_OBJECT header remaining and then over-read the buffer. A collection's trailing free space is marked by an object with index 0, but libhdf5 writes that marker only when at least one 16-byte object header fits. When the remainder is smaller it is undefined padding.

Add tests/test_global_heap.py, which builds raw GCOL collections in memory and reads them through GlobalHeap, deterministically reproducing the crash (a <16-byte trailing pad) and covering the index-0 terminator and exactly-full cases.

Closes #259.

Checklist

  • This pull request has a descriptive title and labels
  • This pull request has a minimal description (most was discussed in the issue, but a two-liner description is still desirable)
  • Unit tests have been added (if codecov test fails)
  • All tests pass

`GlobalHeap.objects` looped `while offset < len(self.heap_data)`, so it entered
the loop with fewer than a full 16-byte `GLOBAL_HEAP_OBJECT` header remaining
and then over-read the buffer. A collection's trailing free space is marked by
an object with index 0, but libhdf5 writes that marker only when at least one
16-byte object header fits. When the remainder is smaller it is undefined
padding.

Add `tests/test_global_heap.py`, which builds raw GCOL collections in
memory and reads them through `GlobalHeap`, deterministically reproducing
the crash (a <16-byte trailing pad) and covering the index-0 terminator
and exactly-full cases.

Fixes NCAS-CMS#259.
@ajelenak ajelenak mentioned this pull request Sep 1, 2026
7 tasks

@davidhassell davidhassell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi Aleksandar,

A discussed on the closed #260, this is the PR to close #259, and it all looks good to me.

It's better that #260 in that it has unit tests; and calculates, rather than assumes, the GLOBAL_HEAP_OBJECT_SIZE.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.62%. Comparing base (667c468) to head (6b673fc).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
+ Coverage   78.48%   78.62%   +0.13%     
==========================================
  Files          15       15              
  Lines        3416     3485      +69     
  Branches      546      555       +9     
==========================================
+ Hits         2681     2740      +59     
- Misses        593      602       +9     
- Partials      142      143       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bnlawrence
bnlawrence merged commit fc523da into NCAS-CMS:main Sep 1, 2026
8 checks passed
@ajelenak

ajelenak commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @davidhassell @bnlawrence!

@ajelenak
ajelenak deleted the fix-global-heap-over-read branch September 1, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error in consolidated_metadata when there are lots of variables

3 participants