Spice::readValue returns garbage when reading off the end of an array valued keyword

[jessemapel]

ISIS version(s) affected: Identified in Dev. Likely present since attached SPICE was added

Description

The Spice::readValue function is responsible for reading values either from the SPICE kernel pool or the attached NaifKeywords group on a cube. When reading from an array value keyword in the SPICE kernel pool, it does not properly check that it found a value at the input index. In the code snippet below it only checks the value of found and does not check the value of numValuesRead. According to the NAIF documentation for gdpool, gcpool, and gipool:

start is the index of the first component of `name' to return.
The index follows the C convention of being 0 based.
If `start' is less than 0, it will be treated as 0. If
`start' is greater than the total number of components
available for `name', no values will be returned (n will
be set to zero). However, `found' will still be set to
SPICETRUE.

we need to check the value of both numValuesRead and found because when reading off the end of an array valued keyword, found will be true and numValuesRead will be 0. It appears that the value we get back is a repeat of the last value in the array.

      // This is the success status of the naif call
      SpiceBoolean found = false;

      // Naif tells us how many values were read, but we always just read one.
      //   Use this variable to make naif happy.
      SpiceInt numValuesRead;

      if (type == SpiceDoubleType) {
        SpiceDouble kernelValue;
        gdpool_c(key.toLatin1().data(), (SpiceInt)index, 1,
                 &numValuesRead, &kernelValue, &found);

        if (found)
          result = kernelValue;
      }
      else if (type == SpiceStringType) {
        char kernelValue[512];
        gcpool_c(key.toLatin1().data(), (SpiceInt)index, 1, sizeof(kernelValue),
                 &numValuesRead, kernelValue, &found);

        if (found)
          result = kernelValue;
      }
      else if (type == SpiceIntType) {
        SpiceInt kernelValue;
        gipool_c(key.toLatin1().data(), (SpiceInt)index, 1, &numValuesRead,
                 &kernelValue, &found);

        if (found)
          result = (int)kernelValue;
      }

How to reproduce

We identified this problem while working with LRO WAC data. If you change line 1111 of spice.cpp to check both returns it causes an error when running spiceinit on LRO WAC data because the distortion model first sets the distortion parameters using the base UV/VIS IK Codes which only have 2 values.

https://github.com/USGS-Astrogeology/ISIS3/blob/dev/isis/src/base/objs/Spice/Spice.cpp#L1111

Possible Solution

All of the checks in spice::readValue need to also check that numValuesRead > 0. For example:

      if (type == SpiceDoubleType) {
        SpiceDouble kernelValue;
        gdpool_c(key.toLatin1().data(), (SpiceInt)index, 1,
                 &numValuesRead, &kernelValue, &found);

        if (found && numValuesRead > 0)
          result = kernelValue;
      }

[jessemapel]

This will likely cause errors when spiceinit'ing some camera models as we were previously just reading garbage and this is now throwing an exception. We should first update those camera models that encounter errors and then fix spice::readValue.

[github-actions]

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.