mlvwm icon indicating copy to clipboard operation
mlvwm copied to clipboard

Fix bugs identified by running clang's `scan-build-16` on the MLVWM source

Open morgant opened this issue 1 year ago • 13 comments

I recently learned about Clang's scan-build tool for identifying bugs and ran the following on my OpenBSD amd64/7.5-stable workstation:

scan-build-16 -o tmp/scan-build make -j4

Which identified 29 bugs, including:

  • (x6) Dereference of null pointer
  • (x1) Garbage return value
  • (x1) Result of operation is garbage or undefined
  • (x1) Uninitialized argument value
  • (x1) Unix API
  • (x1) Memory leak
  • (x2) Allocator sizeof operand mismatch
  • (x14) Dead assignment
  • (x2) Dead nested assignment

mlvwm - scan-build results.pdf

morgant avatar Aug 31 '24 16:08 morgant

I have addressed all the "dead assignments" identified in borders.c, menus.c, and event.c with the exception of baseHeight & baseWidth in event.c's ConstrainSize() as they do appear to be used. For the those, I'm wondering if it's a parsing issue since the formatting is quite wonky and there's a possibility that there are shift-jis characters in there.

morgant avatar Aug 31 '24 17:08 morgant

I'm also skipping the "dead nested assignments" identified in event.c for similar reasons.

morgant avatar Aug 31 '24 17:08 morgant

I have fixed a memory leak in misc.c's ReadIcon() due to a new Icon being allocated before checking to see if the icon file at the path actually exists.

morgant avatar Aug 31 '24 17:08 morgant

I have fixed a NULL value being passed to strcmp() in menus.c's ChangeMenuLabel().

morgant avatar Sep 01 '24 22:09 morgant

I have fixed a possible uninitialized argument value in misc.c's ReadIcon().

morgant avatar Sep 02 '24 04:09 morgant

I have fixed all the potential dereference of null pointer values in menu.c's press_menu() & ChoiseMenu().

morgant avatar Sep 02 '24 15:09 morgant

Fixed a dereference of null pointer in misc.c's RaiseMlvwmWindow().

morgant avatar Sep 02 '24 15:09 morgant

Fixed "allocator sizeof operand mismatch" bugs in config.c & mlvwm.c where calloc() was being used to accidentally allocate memory for a number of MlvwmWindows instead of just a number of pointers to MlvwmWindows for Scr.LastActive. Scr.LastActive is an array of pointers to the "last active" window on each virtual desktop, so there may be multiple if there are multiple virtual desktops.

morgant avatar Sep 02 '24 16:09 morgant

Fixed potential garbage return value in event.c's NextActiveWin(), which is the last bug identified by clang's scan-build that I intend to fix at this time. The remaining "dead assignment" bugs in event.c's ConstrainSize() are not something that I feel need to be addressed.

morgant avatar Sep 02 '24 17:09 morgant

I'm going to test this branch for a bit before merging it.

morgant avatar Sep 02 '24 20:09 morgant

Clang was warning about an unused lp variable in menus.c's RedrawMenuBar() at compile time (not scan-build), so I figured I'd address it here as well.

morgant avatar Sep 03 '24 20:09 morgant

There are a couple more compile warnings that I'd like to address now too (related to Issue #6, replacing sprintf() with snprintf()):

mlvwm.c:386(mlvwm.o:(Done)): warning: strcpy() is almost always misused, please use strlcpy()
functions.c:322(functions.o:(ChangeDesk)): warning: sprintf() is often misused, please use snprintf()

Update: No need to address the second warning about sprintf() in functions.c, I clearly addressed that in a comment on Issue #6. Furthermore, I have split this out into Issue #49 so it can be addressed separately and more appropriately (esp. using libbsd under Linux.)

morgant avatar Sep 03 '24 20:09 morgant

I also took this opportunity to correct the spelling of ChoiseMenu() to ChooseMenu() in functions.c & menus.c.

morgant avatar Sep 03 '24 21:09 morgant