[fitsbits] Possible bug in CFITSIO

[Philip Hodge]

In your example, the FITS image contains 16-bit signed integer values.  
It is not possible for the value to be less than -32768 or greater than 
+32767, so there cannot be an overflow when adding BZERO = 32768.  There 
would be an overflow, however, if BZERO were greater than 32768.  So the 
question is whether CFITSIO checks for that case and returns a float 
instead of an unsigned int with an incorrect value.  Have you checked that?

Phil

On 2/28/20 4:49 AM, David C. Partridge via fitsbits wrote:
>
> External Email - Use Caution
>
> So for clarity:
>
> Are you saying that if the headers of a 16 bit FITS file say:
>
> SIMPLE  =                    T / file does conform to FITS standard
>
> BITPIX  =                   16 / number of bits per data pixel
>
> BZERO   =                32768 / offset data range to that of unsigned 
> short
>
> BSCALE  =                    1 / default scaling factor
>
> that I should ignore the fact the BITPIX is telling me to use signed 
> short, and act as if
>
> BITPIX had a value of 20 (unsigned short).
>
> And similarly 32 bit files with values of BITPX of 32 (signed) and 40 
> (unsigned)??
>
> That somehow feels wrong!
>
> David
>
> *From:*William Pence [mailto:wdpence2000 at yahoo.com]
> *Sent:* 27 February 2020 16:56
> *To:* CCFITS Help Desk
> *Cc:* david.partridge at perdrix.co.uk; fitsbits at listmgr.nrao.edu
> *Subject:* Fwd: [fitsbits] Possible bug in CFITSIO
>
> 
>
> 
>
> 
>
> The HEASARC at NASA/GSFC continues to maintain the CFITSIO library. 
>  Any issues with CFITSIO should be addressed to their help desk at 
> ccfits at heasarc.gsfc.nasa.gov.  I’m forwarding this email to them for 
> possible follow up.
>
> While the topic of how CFITSIO internally handles datatype conversion 
> between every possible FITS datatype and every possible C variable 
> datatype is too complex to go into here, the short answer is that it 
> is being handled correctly.  This particular code has been widely used 
> for the past 25 years, so any serious bugs would surely have been 
> discovered by now.
>
> Bill Pence
>
> HEASRC, NASA/GSFC, Emeritus
>
> ——————————————-
>
>
>
> Begin forwarded message:
>
>     *From:* "David C. Partridge via fitsbits" <fitsbits at listmgr.nrao.edu>
>     *Date:* February 27, 2020 at 6:56:03 AM EST
>     *To:* fitsbits at nrao.edu
>     *Subject:* *[fitsbits] Possible bug in CFITSIO*
>     *Reply-To:* "David C. Partridge" <david.partridge at perdrix.co.uk>
>
>     ===== QUOTE =====
>     Although FITS does not directly support unsigned integers as one
>     of its
>     fundamental data types, FITS can still be used to efficiently
>     store unsigned
>     integer data values in images and binary tables. The convention
>     used in FITS
>     files is to store the unsigned integers as signed integers with an
>     associated offset (specified by the BZERO or TZEROn keyword). For
>     example,
>     to store unsigned 16-bit integer values in a FITS image the image
>     would be
>     defined as a signed 16-bit integer (with BITPIX keyword =
>     SHORT_IMG = 16)
>     with the keywords BSCALE = 1.0 and BZERO = 32768. Thus the
>     unsigned values
>     of 0, 32768, and 65535, for example, are physically stored in the
>     FITS image
>     as -32768, 0, and 32767, respectively; CFITSIO automatically adds
>     the BZERO
>     offset to these values when they are read.
>     ===== END QUOTE =====
>
>     With that in mind I believe that the code in fffi2i2 (for example)
>     at line
>     1054 is incorrect as it always adds the zero value before checking
>     for an
>     overflow error.
>
>     I think that it should check the value is in range and ONLY then
>     add the
>     zero value.
>
>     IOW:
>
>                for (ii = 0; ii < ntodo; ii++)
>                {
>                    dvalue = input[ii] * scale;   // don't do + zero;
>
>                    if (dvalue < DSHRT_MIN)
>                    {
>                        *status = OVERFLOW_ERR;
>                        dvalue = DSHRT_MIN;
>                    }
>                    else if (dvalue > DSHRT_MAX)
>                    {
>                        *status = OVERFLOW_ERR;
>                        dvalue = DSHRT_MAX;
>                    };
>                    dvalue += zero;    // NOW Add the zero offset to
>     the validated
>     value
>                    output[ii] = (short) dvalue;
>                }
>
>     Similar issue with handling of 32 bit values.
>
>     David
>
>
>     _______________________________________________
>     fitsbits mailing list
>     fitsbits at listmgr.nrao.edu
>     https://listmgr.nrao.edu/mailman/listinfo/fitsbits
>
>
> _______________________________________________
> fitsbits mailing list
> fitsbits at listmgr.nrao.edu
> https://listmgr.nrao.edu/mailman/listinfo/fitsbits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listmgr.nrao.edu/pipermail/fitsbits/attachments/20200228/5ba05456/attachment-0001.html>