Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r146898961
--- Diff: src/protocols/vnc/user.c ---
@@ -112,8 +112,10 @@ int guac_vnc_user_leave_handler(guac_user* user) {
guac_vnc_client* vnc_client = (guac_vnc_client*) user->client->data;
- /* Update shared cursor state */
- guac_common_cursor_remove_user(vnc_client->display->cursor, user);
+ if (vnc_client && vnc_client->display && vnc_client->display->cursor) {
--- End diff --
From `guac_common_display_alloc()`, if the display is successfully
allocated at all, it is not possible for the `cursor` to be `NULL`:
https://github.com/apache/incubator-guacamole-server/blob/95be88be19e04e07ac1dafb823993745bee7d146/src/common/display.c#L115-L116
Similarly, the leave handler for the user is assigned only after the
`guac_vnc_client` has been allocated and assigned, thus this function will only
be invoked after `vnc_client` here is known to be non-`NULL`:
https://github.com/apache/incubator-guacamole-server/blob/95be88be19e04e07ac1dafb823993745bee7d146/src/protocols/vnc/client.c#L55
Checking pointers for `NULL` is not something that should be done blanketly
for all pointers just because those pointers are dereferenced. Such checks only
make sense when:
1. `NULL` is one of the expected values of the pointer.
2. It is the responsibility of the function in question to validate the
pointer.
In this case, both of the above are true for the `display` pointer. It is
explicitly initialized to `NULL` due to the `guac_vnc_client` being allocated
using `callloc()`, and given that the display is not guaranteed to be allocated
as written, it's clear that the handlers assigned within `guac_client_init`
need to take this into account before attempting to use the display.
---
