Bug #1058
Use of uninitialised values in GHealpix::dir2pix if m_coordsys == 3 or 4
| Status: | Closed | Start date: | 01/05/2014 | |
|---|---|---|---|---|
| Priority: | Normal | Due date: | ||
| Assigned To: |  Deil Christoph | % Done: | 100% | |
| Category: | - | |||
| Target version: | 00-08-00 | |||
| Duration: |
Description
Just for fun I ran the clang static analyser on gammalib and it found a handful of issues.
http://clang-analyzer.llvm.org
It’s super-simple, just write `scan-build-mp-3.3 ./configure --enable-debug` (available via `sudo port install clang-3.3`)
One is in GHealpix::dir2pix and is described in the attached HTML file.
I think this is a real bug (I did not write a test), when m_coordsys == 3 or 4.
int m_coordsys; //!< 0=celestial, 1=galactic, 2=ecl, 3=hel, 4=sgl
A simple solution to fix this issue would be to throw in the switch default?
But really using integer constants (0, 1, 2, 3, 4) across multiple classes (GHealpix, GSkyProjection) doesn’t seem a good solution.
enum?
report-1cpyQQ.html 
(118 KB)
scan-build-2014-01-06-1.tar.gz (5.01 MB)
Recurrence
No recurrence.
Associated revisions
Remove extra non-implemented coordinate systems from GSkyProjection (fixes #1058).
History
#1
Updated by Deil Christoph over 10 years ago
- File scan-build-2014-01-06-1.tar.gz added
The issues found are:
- Dead assignments in GWcsMER.cpp, GSkymap.cpp
- Dereferences of null pointer in GFitsTableBitCol.cpp, GMatrixSparse.cpp, GFitsTable.cpp, GMatrixSymmetric.cpp, GFitsTableBitCol.cpp
- Double frees in GFitsHeaderCard.cpp
- The uninitialised argument value in GHealpix.cpp mentioned above.
I’m not 100% sure all of those are real issues that can happen, but probably worth a look.
#2
Updated by Deil Christoph over 10 years ago
Pull request for one commit: “Fix some unimportant compiler warnings to have a clean build.”
https://github.com/cdeil/gammalib/commit/b4fb8da329a6c9d94c2553a393dac4ac85506ddc
#3
Updated by Knödlseder Jürgen over 10 years ago
Deil Christoph wrote:
Pull request for one commit: “Fix some unimportant compiler warnings to have a clean build.”
https://github.com/cdeil/gammalib/commit/b4fb8da329a6c9d94c2553a393dac4ac85506ddc
Applied the commit.
#4
Updated by Knödlseder Jürgen over 10 years ago
Deil Christoph wrote:
I've attached the static analyser output report.
The issues found are:
- Dead assignments in GWcsMER.cpp, GSkymap.cpp
Corrected.
- Dereferences of null pointer in GFitsTableBitCol.cpp, GMatrixSparse.cpp, GFitsTable.cpp, GMatrixSymmetric.cpp, GFitsTableBitCol.cpp
Not bugs.
- Double frees in GFitsHeaderCard.cpp
Not bugs.
- The uninitialised argument value in GHealpix.cpp mentioned above.
This is indeed a bug. The HealPix code implements indeed only the 0 and 1 coordinate systems. Probably the >1 coordinate systems should be removed from GSkyProjection as they are not really supported by GammaLib anyways (e.g. GSkyDir does not provide any conversion for these coordinates).
I’m not 100% sure all of those are real issues that can happen, but probably worth a look.
#5
Updated by Deil Christoph over 10 years ago
- Status changed from New to Pull request
Pull request: https://github.com/cdeil/gammalib/compare/gammalib:devel...cdeil:issue_1058
I still think the raw 0 and 1 constants should be replaced by something more readable ... not sure how best to implement it though.- #define, const, enum ?
- Located where?
#6
Updated by Knödlseder Jürgen over 10 years ago
I agree that an enumerator should be used ultimately.
Concerning your changes, I prefer of not changing the default argument of the HEALPix constructor, as some existing code may already rely on this.
#7
Updated by Knödlseder Jürgen over 10 years ago
- Status changed from Pull request to Closed
- % Done changed from 0 to 100
Pushed into devel.
#8
Updated by Deil Christoph over 10 years ago
There’s two reasons why I changed the default coordinate system in the HEALPIX constructor from Galactic to Equatorial:
- There are ambiguities in the definition of Galactic coordinates (see references here: http://docs.astropy.org/en/latest/api/astropy.coordinates.builtin_systems.Galactic.html)
- The default coordinate system in
GSkyProjection, the base class ofGHealpixis equatorial and this default should be consistent within GammaLib:
http://gammalib.sourceforge.net/doxygen/GSkyProjection_8cpp_source.html#l00211
I don’t think backwards compatibility should stop any possible improvements in GammaLib at this very early stage.
#9
Updated by Knödlseder Jürgen over 10 years ago
Deil Christoph wrote:
There’s two reasons why I changed the default coordinate system in the HEALPIX constructor from Galactic to Equatorial:
- There are ambiguities in the definition of Galactic coordinates (see references here: http://docs.astropy.org/en/latest/api/astropy.coordinates.builtin_systems.Galactic.html)
... but we use galactic coordinates widely despite these ambiguities, and it works ...
- The default coordinate system in
GSkyProjection, the base class ofGHealpixis equatorial and this default should be consistent within GammaLib:
http://gammalib.sourceforge.net/doxygen/GSkyProjection_8cpp_source.html#l00211
Good point. I have not thought about this. So I agree to change to EQU 