Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checksum not verified until loading HDUs #8115

Closed
saimn opened this issue Nov 12, 2018 · 2 comments · Fixed by #10012
Closed

Checksum not verified until loading HDUs #8115

saimn opened this issue Nov 12, 2018 · 2 comments · Fixed by #10012

Comments

@saimn
Copy link
Contributor

saimn commented Nov 12, 2018

Since the addition of lazy loading, checksum are verified only for the primary HDU when opening the file, and the other HDUs are verified only when (and if) there are loaded. So if an invalid HDU is not explicitly loaded, it will not be verified.
I think this is wrong, and specifying checksum=True in fits.open should load all HDUs to verify them. The documentation also states that opening is enough to verify the file: http://docs.astropy.org/en/latest/io/fits/usage/verification.html#verification-using-the-fits-checksum-keyword-convention

(example after adding prints in _ValidHDU._verify_checksum_datasum).

In [1]: %astropy
Numpy 1.15.4
Astropy 3.2.dev23320

In [2]: hdu = fits.ImageHDU(data=np.arange(10))

In [3]: hdu.writeto('test.fits', checksum=True, overwrite=True)

In [4]: hdul = fits.open('test.fits', checksum=True)
verify checksum <astropy.io.fits.hdu.image.PrimaryHDU object at 0x7fedcefbf240>
verify datasum <astropy.io.fits.hdu.image.PrimaryHDU object at 0x7fedcefbf240>

In [5]: len(hdul)
verify checksum <astropy.io.fits.hdu.image.ImageHDU object at 0x7fedcefc6320>
verify datasum <astropy.io.fits.hdu.image.ImageHDU object at 0x7fedcefc6320>
Out[5]: 2
@pllim
Copy link
Member

pllim commented Nov 12, 2018

Seems implied but not explicitly stated -- Are you saying that if both checksum and lazy_load_hdus are True, some sort of exception should be raised?

@saimn
Copy link
Contributor Author

saimn commented Nov 12, 2018

No, the code should just force the loading of all HDUs to trigger the verification. That was the default behavior before lazy loading.

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

Successfully merging a pull request may close this issue.

2 participants