Skip to content

Ensure space for the null-terminator to solve heap corruption - #20428

Open
Rob2688 wants to merge 3 commits into
astropy:mainfrom
Rob2688:heapCorruptionFix
Open

Rob2688 wants to merge 3 commits into
astropy:mainfrom
Rob2688:heapCorruptionFix

Conversation

@Rob2688

@Rob2688 Rob2688 commented Sep 17, 2026 •

Copy link
Copy Markdown

Description

This pull request is to address a heap corruption issue
Fix: Ensure to allocate memory for the null operator that is causing the issue.

Fixes #20419

AI Disclosure

AI Tools Disclosure

  • Tool / Model: Gemini (Google)
  • Usage Description: Used to analyze C backtraces, debug pointer arithmetic and buffer reallocation logic in astropy/io/votable/src/tablewriter.c, explain CPython C-API functions (PyArg_ParseTuple, PyUnicode_AsUTF8AndSize), and assist in drafting the regression test and changelog entry.
  • Generated Content: Initial analysis of the *x = (CHAR)0; off-by-one buffer boundary edge case, and draft text for the changelog. All generated code and analysis were manually reviewed, tested, and validated locally.
  • I certify that I am human and that I take full responsibility for this pull request including all interactions with reviewers.

Merge method

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim added this to the v8.1.0 milestone Sep 17, 2026
@pllim pllim added the Bug label Sep 17, 2026
@pllim

pllim commented Sep 17, 2026

Copy link
Copy Markdown
Member

If AI tools were used to develop this pull request, describe the tools including specific model and version, how they were used, and what content is AI generated.

Also need change log and test. Make sure the test segfault without the patch but does not segfault with it.

Also see: https://docs.astropy.org/en/latest/index_dev.html

@pllim

pllim commented Sep 18, 2026

Copy link
Copy Markdown
Member

@Rob2688 please stop adding merge commits to this PR and instead work on resolving remaining asks. Thanks.

@Rob2688

Rob2688 commented Sep 19, 2026

Copy link
Copy Markdown
Author

Hello sorry for the confusion. I'm still new to all this contribution stuff. I changed the log and I'm trying to test. I'll get back to you as soon as possible.

@Rob2688

Rob2688 commented Sep 19, 2026

Copy link
Copy Markdown
Author

I've added the regression test to astropy/io/votable/tests/test_table.py as requested. All tests are passing and this PR is ready for final maintainer review.

@astrofrog

Copy link
Copy Markdown
Member

@Rob2688 can you rebase to get rid of the merge commit?

Comment thread astropy/io/votable/tests/test_table.py Outdated
@pllim

pllim commented Sep 21, 2026

Copy link
Copy Markdown
Member

For change log, instructions at https://github.com/astropy/astropy/blob/main/docs/changes/README.rst . Thanks!

@Rob2688

Rob2688 commented Sep 22, 2026

Copy link
Copy Markdown
Author

@pllim Thank you for your review! I updated the test to use simple strings and added my changelog file

from astropy.utils.misc import _NOT_OVERWRITING_MSG_MATCH


def test_c_tabledata_writer_buffer_overflow():

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.

I am not sure if this test adds any value. It does not fail with astropy 8.0.1 without your proposed patch.

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.

@J-Christophe since you are the one who encountered the problem, are you able to suggest a MWE as regression test?

This branch has not been deployed

No deployments
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.

votable: intermittent SIGABRT / heap corruption in write_tabledata (C TABLEDATA writer)

3 participants