morgant / mlvwm

Macintosh-like Virtual Window Manager (official repo)
http://www2u.biglobe.ne.jp/~y-miyata/mlvwm.html
283 stars 10 forks source link

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

Open morgant opened 3 months ago

morgant commented 3 months ago

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:

mlvwm - scan-build results.pdf

morgant commented 3 months ago

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 commented 3 months ago

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

morgant commented 3 months ago

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 commented 3 months ago

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

morgant commented 3 months ago

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

morgant commented 3 months ago

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

morgant commented 3 months ago

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

morgant commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

morgant commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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