Bug #1058

Use of uninitialised values in GHealpix::dir2pix if m_coordsys == 3 or 4

Added by Deil Christoph almost 11 years ago. Updated almost 11 years ago.

Status:ClosedStart date:01/05/2014
Priority:NormalDue 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 Magnifier (118 KB) Deil Christoph, 01/05/2014 11:57 PM

scan-build-2014-01-06-1.tar.gz (5.01 MB) Deil Christoph, 01/06/2014 11:36 AM

Recurrence

No recurrence.

Associated revisions

Revision 1643814d
Added by Deil Christoph almost 11 years ago

Remove extra non-implemented coordinate systems from GSkyProjection (fixes #1058).

History

#1 Updated by Deil Christoph almost 11 years ago

I've attached the static analyser output report.
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 almost 11 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 almost 11 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 almost 11 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 almost 11 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 almost 11 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 almost 11 years ago

  • Status changed from Pull request to Closed
  • % Done changed from 0 to 100

Pushed into devel.

#8 Updated by Deil Christoph almost 11 years ago

There’s two reasons why I changed the default coordinate system in the HEALPIX constructor from Galactic to Equatorial:

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 almost 11 years ago

Deil Christoph wrote:

There’s two reasons why I changed the default coordinate system in the HEALPIX constructor from Galactic to Equatorial:

... but we use galactic coordinates widely despite these ambiguities, and it works ...

Good point. I have not thought about this. So I agree to change to EQU :)

Also available in: Atom PDF