You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by itsankoff <gi...@git.apache.org> on 2017/10/24 17:22:28 UTC
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
GitHub user itsankoff opened a pull request:
https://github.com/apache/incubator-guacamole-server/pull/120
GUACAMOLE-424: Fix null pointer dereference for vnc client display
More information you can see in the issue description: https://issues.apache.org/jira/browse/GUACAMOLE-424
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/itsankoff/incubator-guacamole-server master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-server/pull/120.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #120
----
commit 15f6c4f3dc51c68e0f869fb13c75c79bd2f0a4db
Author: itsankoff <iv...@gmail.com>
Date: 2017-10-24T17:21:03Z
GUACAMOLE-424: Fix null pointer dereference for vnc client display
----
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by itsankoff <gi...@git.apache.org>.
Github user itsankoff commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r151119494
--- Diff: src/common/display.c ---
@@ -99,6 +99,19 @@ static void guac_common_display_free_layers(guac_common_display_layer* layers,
}
+/**
+ * Allocates a display and a cursor which are used to represent the remote
+ * display and cursor. If the allocation fails then the function returns NULL.
--- End diff --
Same here - done
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by mike-jumper <gi...@git.apache.org>.
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.
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r150634687
--- Diff: src/common/cursor.c ---
@@ -34,6 +34,14 @@
#include <stdlib.h>
#include <string.h>
+
+/**
+ * Allocates a cursor as well as an image buffer where the cursor is rendered.
+ * If the allocation fails then the function returns NULL.
--- End diff --
This is much better, but please be sure to also use the `@return` tag to document the return value/behavior specifically, not just in the overall description of the function. For example:
https://github.com/apache/incubator-guacamole-server/blob/025fc0525f28b05be206d1471e2d8d488f97cb14/src/pulse/pulse/pulse.h#L92-L94
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r150634771
--- Diff: src/common/display.c ---
@@ -99,6 +99,19 @@ static void guac_common_display_free_layers(guac_common_display_layer* layers,
}
+/**
+ * Allocates a display and a cursor which are used to represent the remote
+ * display and cursor. If the allocation fails then the function returns NULL.
--- End diff --
Same here - much better, but return values should also be documented with `@return`.
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r146633955
--- 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 --
Without justification, this check is too broad. Have you verified which of these pointers is the one that is potentially invalid? For whichever pointer that is, it known that the value will indeed be `NULL` if not valid?
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by itsankoff <gi...@git.apache.org>.
Github user itsankoff commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r151119382
--- Diff: src/common/cursor.c ---
@@ -34,6 +34,14 @@
#include <stdlib.h>
#include <string.h>
+
+/**
+ * Allocates a cursor as well as an image buffer where the cursor is rendered.
+ * If the allocation fails then the function returns NULL.
--- End diff --
:+1: Done
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r148563508
--- 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 --
I'd say go ahead and update with a commit to remove the vnc_client check but keep the other two. Mike can weigh in on checking cursor and then we can get this merged.
Thanks!
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-server/pull/120
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by itsankoff <gi...@git.apache.org>.
Github user itsankoff commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r148159347
--- 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 --
Yeah I totally agree about blindly checking for NULL values. I also agree about the `vnc_client` check and I will remove it. But for the `cursor` there is actually a rare case when cursor allocation can fail and cursor within a valid display to be `NULL` (https://github.com/apache/incubator-guacamole-server/blob/95be88be19e04e07ac1dafb823993745bee7d146/src/common/cursor.c#L40-L41). That's why I think with these thoughts in mind the final check should be:
```
if (vnc_client->display && vnc_client->display->cursor) {
...
}
```
---
[GitHub] incubator-guacamole-server pull request #120: GUACAMOLE-424: Fix null pointe...
Posted by itsankoff <gi...@git.apache.org>.
Github user itsankoff commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/120#discussion_r146669866
--- 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 --
Yeah I know exactly which. The problem is that the display pointer is null. When I traced it, every time the server crashed the `display` pointer was `NULL`. My first modification was to check against `NULL` only for it, but then I thought it makes sense to check if we `vnc_client` is not `NULL` pointer before check for the `display`. And also I saw that ` guac_common_cursor_remove_user(vnc_client->display->cursor, user);` does not check when it dereference the cursor pointer.
---