snprintf to undersized buffer in ffpkyt

[pkgw]

On recent Mac builds, I find that TestProg crashes with an illegal instruction that turns out to be diagnosing an snprintf to an undersized buffer.

The function ffpkyt calls ffd2f with a buffer variable called fstring that's 20 bytes in size. But ffd2f does an snprintf() into this buffer with the size parameter set to FLEN_VALUE, which is set to 71. On Linux it seems that you can get away with this, but on at least some Mac builds, this leads to a crash.

The most straightforward solution is simply to change the size of the fstring buffer.

I haven't checked whether there are other parts of the code demonstrating the same issue, but in my test build this is the only time I see this crash, so if they're there, they're probably pretty obscure.

[c181gordon]

Thanks for sending us the report. But I've been unable to repeat the error even on Mac Sequoia w/ clang 16.0. Can you please provide the specific error message you're seeing? In analyzing the ffpkyt code, I don't see how it's exceeding the 20 byte limit when it calls ffd2f. It's passing a fraction value < 1, with a 16 digit limit in the output. Are you seeing a compile-time warning or error for this?

[pkgw]

This was manifesting as a runtime "illegal instruction" error in the conda-forge builds of cfitsio, during running of TestProg:

/Users/runner/miniforge3/conda-bld/cfitsio_1734902931920/work/conda_build.sh: line 38: 25141 Illegal instruction: 4  ./TestProg > testprog.lis

Log file here although it will disappear in 30 days or so.

Using lldb, I was able to see that the SIGILL was happening in a function named something like _snprintf_overflow_chk. I don't know if this is something that only gets compiled in with certain debug flags, or what. From tracing the execution I could tell that the buffer wasn't actually getting overflowed, but clearly there was some sort of safety check that was killing things before the snprintf() got a chance to even do its work. In this case, the maximum output length could never overflow the buffer in practice, but it doesn't seem too unreasonable for a libc to enforce that the buffer size must be at least as big as the number passed to snprintf().

[esabol]

Whether there is a runtime crash or a compiler error or not is kind of irrelevant. Just by inspection, it's obvious that there's a problem with the ffpkyt code.

As @pkgw said, ffd2f has a snprintf which specifies FLEN_VALUE as the maximum length:

cfitsio/putkey.c

Line 3390 in 26a92a2

if (snprintf(cval, FLEN_VALUE,"%.*f", decim, dval) < 0)

cfitsio/fitsio.h

Line 217 in 26a92a2

#define FLEN_VALUE 71 /* max length of a keyword value string */

But the fstring that ffpkyt passes to ffd2f only allocates 20 characters:

cfitsio/putkey.c

Line 1057 in 26a92a2

char fstring[20], *cptr;

Any good static code analyzer should flag this as a problem with the code. I recommend activating and using GitHub's CodeQL, for example. I'll try to do that in a fork if nobody beats me to it.

[c181gordon]

OK thanks @pkgw and @esabol, this makes sense to me now. So the snprintf overflow check must be flagging any case where the size argument (FLEN_VALUE) is larger than the size of input char array. And it does this even though ffpkyt itself ensures that the combination of 'decim' and 'dval' won't actually produce an overflow in practice.

We can certainly make the change that expands the size of 'fstring' to FLEN_VALUE to remove this problem. I checked all the other calls to ffd2f in cfitsio, and they're all passing in char arrays of size FLEN_VALUE. Thanks for bringing this to our attention and please let us know if you find any other issues.