fix(fits): Fix recursion stack overflow with too many header blocks#5248
Conversation
FITS files are allowed to have multiple "header blocks". Unfortunately, as originally written, our fits input used *recursion* to read them. If there is a file with too many headers, it can crash with a stack overflow. Change to use a loop instead of recursion. And put a maximum iteration limit of 10000 headers in one file before giving up. Signed-off-by: Larry Gritz <lg@larrygritz.com>
jessey-git
left a comment
There was a problem hiding this comment.
So this change all seems ok. But I'll note that I haven't had much luck getting OIIO to properly display FITS from some random nasa sample images found online. No failures, just bogus images which are already present outside of this PR. One other viewer I have seems to display the same files ok so seems like this format needs more TLC afterwards probably.
As for whitespace, try using the setting here:
I have a soft spot for FITS because I had some early career experience with astronomical image processing. But I don't come across or use them regularly, so I wouldn't be surprised if OIIO's support isn't very complete. Point me to any image that doesn't work and some time when I'm looking for something to keep busy with, I'll try to fix it. (A bigger task would be to use CFITSIO as a dependency and know we have full, robust coverage of the format.)
Thanks! |
|
Errm, actually I think the images are being read correctly now that I look a lot closer (by really stretching the image by lowering the white levels more than I did at first). Though the images still seem a lot dimmer than expected, and I needed to output EXR as the quick PNG check I did before wasn't useful at all. I ended up using the "cntr" and "full" files from inside https://cxc.cfa.harvard.edu/cdaftp/science/ao06/cat6/5926/primary/ (remove the .gz extension as they're just fits). These are "boring" files regardless so expect no wow factor :) |
|
Is it possible that OIIO is assuming that integer values are normalized to the max for the number of bits (e.g. 65535 max for 16 bit unsigned values), but many other FITS readers are perhaps assuming a differing "max", or are even by default doing a histogram stretch for viewing? (Which would make sense for viewing an astro image, which is in a sense data values rather than a visible light image that's meant to replicate our senstation from looking at the "real" scene, such as you'd want for a photo or a render.) Also, I wonder if we're assuming linear values but many other FITS viewers are by default doing a gamma correction or something? |
FITS files are allowed to have multiple "header blocks".
Unfortunately, as originally written, our fits input used recursion to read them. If there is a file with too many header blocks, it can crash with a stack overflow.
Change to use a loop instead of recursion. And put a maximum iteration limit of 10000 headers in one file before giving up.
(Note to reviewers: the diff looks bigger than it really is. By surrounding the contents of read_fits_header with a
forloop, it reindented it, and that is the vast majority of the change. I wish GitHub review diffs more clearly visualized which changed lines are only whitespace changes.)