Fix invalid read of size 1 error from Valgrind.

[Quipyowert2]

After __handle_bpress_action is called, the window fw might have been
freed by CMD_close. Check that the window still exists before checking
if it is scheduled to be raised, so that we don't attempt to read freed memory.

• What does this PR do?
  Fixes an attempt to read freed memory. (Invalid read of size 1 (reading freed memory) #418)

• Screenshots (if applicable)

• PR acceptance criteria (reminder only, please delete once read)

  • Your commit message(s) are descriptive. See:

    https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

  • [https://raw.githubusercontent.com/fvwmorg/fvwm3/master/doc/README]Documentation updated (where appropriate)

  • Style guide followed (try and match the surrounding code where possible)

  • All tests are passing: although this is automatic, Codacy will often
    highlight additional considerations which will need to be addressed before
    the PR can be merged.

• Issue number(s)

If this PR addresses any issues, please ensure the appropriate commit
message(s) contains:

Fixes #XXX

at the end of your commit message, where XXX should be replaced with the
relevant issue number.

If there is more than one issue fixed then use:

Fixes #XXX, fixes #YYY, fixes #ZZZ

[ThomasAdam]

Since check_if_fvwm_window_exists() just walks the FvwmWindow structure looking for itself, we don't need to do this, and can instead just check what fw is, as in:

if (fw != NULL && IS_SCHEDULED_FOR_RAISE(fw))

[Quipyowert2]

I compiled and ran fvwm3 from master branch (356e327) with the vgdb gdb server (valgrind --vgdb-error=0 fvwm/fvwm3 and then in another terminal gdb -ex 'target remote | vgdb' and then continue past the uninitialized value errors) and after Valgrind prints the Invalid read of size 1 error (which happens when closing a window) gdb says fw is 0x947bd60 and monitor get_vbits 0x947bd60 (which asks Valgrind about the validity of an address) says that fw is unaddressable.

I don't think a NULL check would fix the problem, because fw has been freed but hasn't been set to NULL, so it's a dangling pointer. Another way to solve this would be to use a function that takes a pointer to pointer and free the pointed-to pointer and then set it to NULL. Something like this mem_dealloc_ function.

destroy_window() seems to remove fw from the window list at addwindow.c:3367. If Scr.FvwmRoot in check_if_fvwm_window_exists is part of the same linked list then fw won't be in the list so the condition at line 1727 of events.c will short-circuit and not check IS_SCHEDULED_FOR_RAISE(fw) thereby avoiding the use-after-free.

Valgrind doesn't report the invalid read error if I compile fvwm3 from my branch nm/fix-invalid-read and repro with the steps at issue #418.

I had to recompile events.c with -O0 to get gdb to tell me the actual value of fw instead of saying <optimized out>

GDB output after valgrind hits the invalid read error with master branch (356e327 ) of fvwm3:

Program received signal SIGTRAP, Trace/breakpoint trap.
0x000000000042b8dd in __handle_bpress_on_managed (exc=0x948ed70) at events.c:1727  
1727            if (IS_SCHEDULED_FOR_RAISE(fw))
(gdb) print fw
$1 = (FvwmWindow * const) 0x947bd60
(gdb) monitor get_vbits 0x947bd60
__ 
Address 0x947BD60 len 1 has 1 bytes unaddressable 
(gdb)

[ThomasAdam]

Hi @Quipyowert2

Thanks. Please can you include this in your ammended commit message in your PR, and I'll approve it. Well spotted!

[ThomasAdam]

Hi @Quipyowert2

Thanks. This seems like a false positive to me. Before calling __handle_bpress_on_managed() we've already checked to see if the window is valid (i.e., not NULL). For it to go between then, and raising the window, IS_SCHEDULED_FOR_DESTROY would have to happen.

Since it's very unlikely, we can still leave a check in, I suppose. I've left a suggestion at the appropriate line.

[ThomasAdam]

Thanks again, @Quipyowert2.