Static analyzer issues

[shlyakpavel]

Hello. Have been walking around X11 driver with clang-tidy. I'll comment only the places I suppose to have mistakes
The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage https://github.com/fltk/fltk/blob/master/src/drivers/X11/Fl_X11_Screen_Driver.cxx#L971
Ints are used without initialization here. Looks weird. https://github.com/fltk/fltk/blob/master/src/drivers/X11/Fl_X11_Window_Driver.cxx#L237
P is initialized but never used. It is probably not a bug, but still suspicious. https://github.com/fltk/fltk/blob/master/src/drivers/X11/Fl_X11_Screen_Driver.cxx#L1241
In general, the whole project should be reviewed with a static analyzer as there are 572 problems found with only strict options applied. I guess it is so for the long FLTK history, but still...

[Albrecht-S]

Thanks for your report and the heads-up to use a static analyzer like clang-tidy (edited, was: -format). As for your reports:

1. https://github.com/fltk/fltk/blob/master/src/drivers/X11/Fl_X11_Screen_Driver.cxx#L971 : I think this is one for our specialist @ManoloFLTK.
2. https://github.com/fltk/fltk/blob/master/src/drivers/X11/Fl_X11_Window_Driver.cxx#L237 : This is not a fault. X, Y, W, and H are reference arguments that return values. Yes, we could initialize them with 0 to prevent warnings. I'll take a look...
3. https://github.com/fltk/fltk/blob/master/src/drivers/X11/Fl_X11_Screen_Driver.cxx#L1241 : yes, you are right, it's not a bug. I think the following change would make it clear:

  p = fgets(line, sizeof(line), in);
  if (p && strstr(line, "<monitors version=\"2\">")) {

i.e. add p && in the condition. I'll take this one as well.

PS: I checked the entire source with clang-tidy (default, no special setup), and I found some issues. It doesn't look too bad but there are some noteworthy warnings I'll check and take care of. Thanks again.

[Albrecht-S]

Part 3 is fixed in commit 2e6a4eb.
Part 2 needs a little more work since there are many similar places in the code.

[Albrecht-S]

Regarding part 1:

The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage. https://github.com/fltk/fltk/blob/master/src/drivers/X11/Fl_X11_Screen_Driver.cxx#L971

I can see and fix the issue, but unfortunately my version of clang-tidy (6.0) doesn't issue this warning. I'm using:
clang-tidy $1 -- -I$SRCDIR -DFL_LIBRARY `$BLDDIR/fltk-config --use-images --use-gl --cxxflags`
where $1 is the filename, $SRCDIR is the FLTK source directory, and $BLDDIR is the FLTK build directory which may be different if using CMake. I'm using the default config.

Pavel, can you please elaborate on how you are calling clang-tidy?

[shlyakpavel]

@Albrecht-S I'm not sure about the flags as I use Qt Creator to develop. It has predefined sets of flags, so I used one of these...

[Albrecht-S]

@shlyakpavel Thanks for your reply anyway. Do you know your clang and clang-tidy versions?

[shlyakpavel]

@Albrecht-S

./clang --version
clang version 7.0.0 (git://code.qt.io/clang/clang.git 1817513d4f3a2e4e26be124dbe395340f798fd51) (git://code.qt.io/clang/llvm.git 287d3ae13f372cfa7f30801199d02a99b60d563b)
Target: x86_64-unknown-linux-gnu
Thread model: posix

clang-tidy mask was -*,clang-analyzer-*

[shlyakpavel]

However, I personally prefer to use PVS studio instead of clang-tidy. According to my experience it always finds much more relevant problems than clang-based tools do. They are commercial project but one can ask 'em for a trial license :)

[erco77]

Apparently PVS Studio has a command line as well, allowing for automation (and avoiding GUI IDEs):
https://www.viva64.com/en/m/0035/

[ManoloFLTK]

Regarding part 1:

The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage. https://github.com/fltk/fltk/blob/master/src/drivers/X11/Fl_X11_Screen_Driver.cxx#L971

That is May 2002 code by M.R.Sweet which decodes the output of the XGetImage
Xlib function into an RGB byte array. I have no knowledge of the algorithm which seems
somewhat complex. The statements containing index_mask at lines #971 and #974
apparently should just be removed because the value of that variable isn't used anywhere
in the program path that goes through these lines. Albrecht: is that your finding too?

[Albrecht-S]

No, I don't think so. I'd rather believe that the correct fix would be to add an initialization in the loop statement at line no. 961 like this one:

          for (x = image->width, line_ptr = line, index_mask = 192, index_shift = 6;

This seems to be what is used inside the loop. But I'd need to check this - it's just my impression from looking at the code...

@ManoloFLTK Why did you close the issue? By accident? Sorry for mentioning you explicitly and for assigning the issue to you. I'll look into it and see if the above fix is correct. Feel free to unassign yourself.

[Albrecht-S]

@ManoloFLTK Sorry again, your proposal is probably correct since the index_mask value is never used.

Still looking into it...

[ManoloFLTK]

 @ManoloFLTK Why did you close the issue? By accident? 

That must have been an accident, while discovering github.

[Albrecht-S]

OK, part 1 is now also fixed in commit 59cd39a.
I removed the questionable variable in this case statement since it is not used as pointed out by @ManoloFLTK. Thanks.

[Albrecht-S]

Regarding part 2 (uninitialized ints): I opened a new issue (#6) with a more general description of the issue.

I'm closing this one now. Thanks for all contributions.

[fab672000]

Could be nice to run a static analyzer after each submission / pr request to master ...