Starlink / starlink

Starlink Software Collection
162 stars 53 forks source link

AST library fails to parse floating point values in FITS headers if locale uses commas as a decimal separator #53

Open confluence opened 8 years ago

confluence commented 8 years ago

I have recently encountered this issue on Ubuntu 14.04 with the locale set to en_ZA.UTF-8, while using software which uses the AST library as a dependency.

This happens because the native sscanf which AST uses to parse the header is locale-aware. I can see that AST provides its own replacement function which it substitutes on some platforms; however, it was not selected on my platform when I installed AST (8.0.7 and several older versions) from the source tarball.

In addition to this, when this error is encountered, AST attempts to print the affected card in an error message, but does not truncate the char array after 80 characters, so it actually attempts to print the entire remainder of the header, which causes a buffer overflow.

Setting the LC_NUMERIC environment variable to e.g. C fixes my problem locally. Is this the correct way of fixing the problem, or should AST use its replacement function on platforms where it detects a locale with a nonstandard separator? My understanding is that FITS files are not locale-aware and should always use the decimal point as a separator, in which case AST's current behaviour seems to be a bug.

dsberry commented 8 years ago

Thanks for the detailed bug report. Locale is something we've not thought about before with AST. It seems fairly clear that when reading or writing values through any form of an AST Channel (a FitsChan for instance), the C locale should be used internally within AST. But what is not so clear is how to handle values supplied as string arguments to functions such as astSetC . For these cases, AST doesn't know where the supplied strings came from originally, and so can't tell what locale they use. The same applies to strings returned by astGetC. Maybe it is then the responsibility of the calling code to set the correct locale prior to calling AST.

confluence commented 8 years ago

I'm involved in the development of the code in question, so I will suggest this to the rest of the team. It's a small change, and I have verified that it works.

timj commented 8 years ago

I think it's clear that for parsing of FITS headers we have to be using the C locale as that is essentially how the FITS standard is defined.

dsberry commented 8 years ago

And also when parsing text created by an astChannel (i.e. native AST representation). But the question is what to do about text supplied via other means like astSetC.

confluence commented 8 years ago

To clarify, our code is using a FitsChan, so it seems like a clear-cut case where the locale should be fixed to C. I have implemented a workaround by setting the locale in our code, at least temporarily.