Fix ROBOSPECT to be compatible with GSL 2.x#2
Open
gszasz wants to merge 4 commits intoczwa:masterfrom
Open
Conversation
GSL 2.0 broke backward compatibility with GSL 1.x code, which prevented ROBOSPECT from being used with modern Linux distributions. The fix was relatively easy. The only relevant difference that affected the ROBOSPECT code was the need to calculate the Jacobian matrix explicitly via the gsl_mulitifit_fdfsolver_jac() function.
gszasz
changed the title
gszasz
changed the title
The compiler shows several warnings which should be addressed:
fileio.c:5: Pointer to the local statically allocated char array cannot
be returned from the function, because once the program leaves the scope
of the function, there is no guarantee that the array will be preserved.
Making the variable static is the easiest fix of this issue.
fileio/input_fitsWCS.c:93: The k variable can contain a signed integer
number (from -2147483646 to 2147483646) that will occupy between 3 and
11 bytes when formatted into a %03d string. Note that the %03d format
specifies the *minimum* number of digits, but it does not prevent from
including number greater than 999. The snprintf() function then does
the truncation, and thus prevents buffer overflow, but this is generally
quite a bad practice. In this scenario the -Wformat-truncation compiler
argument generates the warning if a developer does not at least check if
the return value of snprintf() is not negative.
fileio/input_fitsWCS:97: This is one of the most dangerous pieces of
code within the ROBOSPECT that is most likely responsible for
unpredictable behavior. The sprintf() function simply cannot read and
write into the same string at the same time. Furthermore, space (' ')
as a flag of the format string do not have any function in "%s" format
and it is simply ignored by sprintf(). I have rewritten this code using
pointer arithmetic to avoid frequent NULL pointer checking.
models/continuum_blackbody.c:76: The undeclared chi variable contains a
random value during the first log_comment() call. Although it is not
critical, accessing undeclared variable should be avoided in any
circumstances. The iterations starts with chi square value 99e99 and
this is exactly what we should be printed during the first log_comment()
call.
The return value of the malloc() and realloc() calls was never checked for an error. Although this did not produce any warning, it is generally a very bad practice. My implementation of the memory allocation error handling might be simplistic, but it represents the bare minimum that should have been implemented a long ago.
Lukas Kueß (@iamasciencer) has encountered enigmatic buffer overflow errors when testing ROBOSPECT on Ubuntu 24.02 LTS. It turned out that Ubuntu 24.02 LTS uses fortified strings by default and since the ROBOSPECT's string memory allocation handling is suboptimal, it did not comply the strict rules of fortified strings. I had to rewrite several parts of config.c to make this work properly.
Open
|
I used to hit issue #1, and this PR fixed it for me. I tested it on Fedora Linux 41 Workstation Edition (x86_64 architecture), and it works without any issues. The code looks good to me. |
|
I ran into buffer overflows on Ubuntu 24.04.1 (64 bit), the patches addressed this issue ant it works now without problems |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR is addressing the note mentioned in the second paragraph of the ROBOSPECT webpage:
The
libgsl0-devpackage mentioned above, containing GSL 1.16, was indeed shipped with Ubuntu 14.04 LTS, released on April 17, 2014. Even though the extended standard support of Ubuntu 14.04 LTS was prolonged to 10 years in September 2021, it reached its end of life on April 17, 2024. It means that over the past 8 months, nobody could use ROBOSPECT in a supported environment. At the same time, the project development has stopped and the promised rewritten newer version of ROBOSPECT has never been delivered.@czwa decided to transfer ROBOSPECT from its original private CSV repo to GitHub on August 28, 2018, allowing the community to continue in his work; nevertheless, no progress was done over the past 6 years.
@pavolgaj filed the issue #1 on December 20, 2021, pinpointing the exact design change in GSL 2.0 that broke the backward compatibility in 2013, asking for advice on how to fix it. Nobody addressed the issue for the following 3 years.
Our Open Cluster Working Group, led by @epaunzen, actively uses the ROBOSPECT code for years, especially within the research projects conducted by @PraptiMondal and @iamasciencer. With the Ubuntu 14.04 LTS reaching its end of life, we have decided to step in.
Description
Our goal was to resolve #1 filed by @pavolgaj, who found out that starting from GSL 2.0, the following code is no longer valid, since the Jacobian matrix is no longer part of the
gsl_multifit_fdfsolverstructure:It turned out that the Jacobian needs to be allocated and defined separately. The
gsl_mulitifit_fdfsolver_jac()is a new function that was introduced in GSL 2.0.We also resolved all warnings reported by
gccto simplify testing. These warnings turned out to be caused with the minor bugs clustered around the following four lines of code:chararray cannot be returned from the function, because once the program leaves the scope of the function, there is no guarantee that the array will be preserved.kvariable can contain a signed integer number (from -2147483646 to 2147483646) that will occupy between 3 and 11 bytes when formatted into a%03dstring. Note that the%03dformat specifies the minimum number of digits, but it does not prevent from including a number greater than 999. Thesnprintf()function then does the truncation, and thus prevents buffer overflow, but this is generally bad coding style. In this scenario, the-Wformat-truncationcompiler argument generates the warning, if a developer does not check the return value ofsnprintf().sprintf()function simply cannot read and write into the same string at the same time. Furthermore, the space character as a flag of the format string does not have any role in the%sformat, and it is ignored bysprintf().chivariable contains a random value during the firstlog_comment()call.Additionally, we found out that Ubuntu 24.04 LTS uses
string_fortified.hinstead ofstring.hand thus it is more sensitive to buffer overflow errors inconfig.c, which needed to be fixed before testing the patch in this environment.We also added preventive checks of the return value of
malloc()andrealloc()calls across the entire ROBOSPECT code.Test Results