Skip to content

Fix a crash that occurred when trying to write out a FITS file to a new file if it was originally read with denywrite#19952

Closed
astrofrog wants to merge 1 commit into
astropy:mainfrom
astrofrog:fix-writeto-readonly-buffer
Closed

Fix a crash that occurred when trying to write out a FITS file to a new file if it was originally read with denywrite#19952
astrofrog wants to merge 1 commit into
astropy:mainfrom
astrofrog:fix-writeto-readonly-buffer

Conversation

@astrofrog

@astrofrog astrofrog commented Jun 19, 2026 •
edited by pllim
Loading

Copy link
Copy Markdown
Member

If a FITS file is read in with denywrite and the data is then written out to a different file, a crash can occur if some columns are being scaled or updated at write-time:

In [4]: import numpy as np
   ...: from astropy.io import fits
   ...: 
   ...: col = fits.Column(name="x", format="E", array=np.array([1.0, 2.0, 3.0]),
   ...:                   bscale=2.0, bzero=5.0)
   ...: fits.BinTableHDU.from_columns([col]).writeto('data.fits', overwrite=True)
   ...: 
   ...: with fits.open('data.fits', mode="denywrite") as hdul:
   ...:    hdul[1].data["x"]
   ...:    hdul.writeto('data2.fits', overwrite=True)
   ...: 
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[4], line 10
      8 with fits.open('data.fits', mode="denywrite") as hdul:
      9    hdul[1].data["x"]
---> 10    hdul.writeto('data2.fits', overwrite=True)  # ValueError: assignment destination is read-only

File ~/Code/Astropy/astropy/astropy/io/fits/hdu/hdulist.py:1066, in HDUList.writeto(self, fileobj, output_verify, overwrite, checksum)
   1064 for hdu in self:
   1065     hdu._output_checksum = checksum
-> 1066     hdu._prewriteto()
   1067     hdu._writeto(hdulist._file)
   1068     hdu._postwriteto()

File ~/Code/Astropy/astropy/astropy/io/fits/hdu/table.py:527, in _TableBaseHDU._prewriteto(self, inplace)
    525 def _prewriteto(self, inplace=False):
    526     if self._has_data:
--> 527         self.data._scale_back(update_heap_pointers=not self._manages_own_heap)
    528         # check TFIELDS and NAXIS2
    529         self._header["TFIELDS"] = len(self.data._coldefs)

File ~/Code/Astropy/astropy/astropy/io/fits/fitsrec.py:1340, in FITS_rec._scale_back(self, update_heap_pointers)
   1337     dummy = np.around(dummy)
   1339 if raw_field.shape == dummy.shape:
-> 1340     raw_field[:] = dummy
   1341 else:
   1342     # Reshaping the data is necessary in cases where the
   1343     # TDIMn keyword was used to shape a column's entries
   1344     # into arrays
   1345     raw_field[:] = dummy.ravel().view(raw_field.dtype)

ValueError: assignment destination is read-only

This PR ensures that we copy the buffer before modifying if needed.

Description

Closes #19955

  • 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.

@astrofrog astrofrog added this to the v7.2.2 milestone Jun 19, 2026
@astrofrog astrofrog requested a review from saimn as a code owner June 19, 2026 12:42
@astrofrog astrofrog added io.fits Bug backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x labels Jun 19, 2026
@github-actions

github-actions Bot commented Jun 19, 2026

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.

@astrofrog
Copy a table's read-only data buffer before scaling back so writing a…
9d567be 
… denywrite-opened table to a new file no longer fails
@astrofrog astrofrog force-pushed the fix-writeto-readonly-buffer branch from 1997e3a to 9d567be Compare June 19, 2026 12:54
@astrofrog astrofrog mentioned this pull request Jun 19, 2026
Draft
1 task
saimn
saimn reviewed Jun 19, 2026
View reviewed changes
Comment thread astropy/io/fits/hdu/table.py
# read-only buffer (e.g. a ``denywrite`` table written to a new file),
# so copy first; own-heap HDUs (compression) don't rewrite here.
if not self._manages_own_heap and not self.data.flags.writeable:
self.data = self.data.copy()

@saimn saimn Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The drawback of this is that people might loose the reference to data if they have one ? Not sure if there is a better thing to do though ...
If the array cannot be made writeable inplace (not sure if this is possible with the underlying mmap), maybe we can also just raise an error telling users that denywrite is not compatible with an array that needs to be modified ?

@astrofrog

astrofrog commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@saimn - your comment prompted me to think of what I think is a better solution here, I'll open a parallel PR

@astrofrog astrofrog mentioned this pull request Jun 19, 2026
Merged
@pllim

This comment was marked as resolved.

Sign in to view
@astrofrog astrofrog removed backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x labels Jun 23, 2026
@astrofrog

astrofrog commented Jun 23, 2026

Copy link
Copy Markdown
Member Author

No I've updated the labels

@pllim pllim modified the milestones: v7.2.2, v8.1.0 Jun 23, 2026
@saimn saimn closed this in #19955 Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saimn saimn saimn left review comments

Assignees

No one assigned

Labels

Bug io.fits

Projects

None yet

Milestone

v8.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@astrofrog @pllim @saimn