Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-518

Image reading should reject conversions that could overflow

    XMLWordPrintable

Details

    • RFC
    • Status: Implemented
    • Resolution: Done
    • DM
    • None

    Description

      While refactoring our FITS reading code as part of DM-15500, I noticed that we currently permit reading into images with pixel types that cannot represent all values of the on-disk type (for example, we allow an on-disk int64 image to be read in as a uint16 in-memory image). It's not clear to me from looking at the code whether this was intentional, and it seems a bit dangerous.

      Instead, I propose we only support the following conversions:

      • any integer or floating point -> any floating-point (potential loss of precision, but no overflow)
      • any unsigned integer -> any larger signed integer
      • any signed integer -> any not-smaller signed integer
      • any unsigned integer -> any not-smaller unsigned integer

      Please note that I am proposing this change just because avoiding overflow seems like the right thing to do; if there is a use case for the current behavior, I'm happy to retain it. I do see lots of tests in afw using conversions that could overflow, but I haven't yet seen a case where it looks anything other than incidental.

      We could also go a bit further and prohibit conversions that would entail a loss of precision, but it's much easier for me to imagine valid use cases for those conversions.

      Attachments

        Issue Links

          Activity

            jbosch Jim Bosch created issue -
            jbosch Jim Bosch made changes -
            Field Original Value New Value
            Risk Score 0
            jbosch Jim Bosch made changes -
            Description While refactoring our FITS reading code as part of DM-15500, I noticed that we currently permit reading into images with pixel types that cannot represent all values of the on-disk type (for example, we allow an on-disk int64 image to be read in as a uint16 in-memory image). It's not clear to me from looking at the code whether this was intentional, and it seems a bit dangerous.

            Instead, I propose we only support the following conversions:
             - any integer or floating point -> any floating-point (potential loss of precision, but no overflow)
             - any unsigned integer -> any larger signed integer
             - any signed integer -> any not-smaller signed integer
             - any unsigned integer -> any not-smaller unsigned integer

            Please note that I am proposing this change just because avoiding overflow seems like the right thing to do; if there is a use case for the current behavior, I'm happy to retain it. I do see lots of tests in afw using conversions, but I haven't yet seen a case where it looks anything other than incidental.

            We could also go a bit further and prohibit conversions that would entail a loss of precision, but it's much easier for me to imagine valid use cases for those conversions.
            While refactoring our FITS reading code as part of DM-15500, I noticed that we currently permit reading into images with pixel types that cannot represent all values of the on-disk type (for example, we allow an on-disk int64 image to be read in as a uint16 in-memory image). It's not clear to me from looking at the code whether this was intentional, and it seems a bit dangerous.

            Instead, I propose we only support the following conversions:
             - any integer or floating point -> any floating-point (potential loss of precision, but no overflow)
             - any unsigned integer -> any larger signed integer
             - any signed integer -> any not-smaller signed integer
             - any unsigned integer -> any not-smaller unsigned integer

            Please note that I am proposing this change just because avoiding overflow seems like the right thing to do; if there is a use case for the current behavior, I'm happy to retain it. I do see lots of tests in afw using conversions that could overflow, but I haven't yet seen a case where it looks anything other than incidental.

            We could also go a bit further and prohibit conversions that would entail a loss of precision, but it's much easier for me to imagine valid use cases for those conversions.
            jbosch Jim Bosch made changes -
            Status Proposed [ 10805 ] Adopted [ 10806 ]
            jbosch Jim Bosch made changes -
            Link This issue is triggering DM-15500 [ DM-15500 ]
            jbosch Jim Bosch made changes -
            Resolution Done [ 10000 ]
            Status Adopted [ 10806 ] Implemented [ 11105 ]

            People

              jbosch Jim Bosch
              jbosch Jim Bosch
              Jim Bosch, John Parejko, John Swinbank, Kian-Tat Lim, Paul Price, Robert Lupton, Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                Jenkins

                  No builds found.