You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by chipitsine <gi...@git.apache.org> on 2017/09/24 08:17:32 UTC
[GitHub] incubator-guacamole-server pull request #109: GUACAMOLE-388: resolve null po...
GitHub user chipitsine opened a pull request:
https://github.com/apache/incubator-guacamole-server/pull/109
GUACAMOLE-388: resolve null pointer dereference
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chipitsine/incubator-guacamole-server master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-server/pull/109.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 #109
----
commit b4d700f958ff7275c9880e97d4debe554c1c1594
Author: Ilya Shipitsin <ch...@gmail.com>
Date: 2017-09-24T08:16:06Z
GUACAMOLE-388: resolve null pointer dereference
----
---
[GitHub] incubator-guacamole-server pull request #109: GUACAMOLE-388: resolve null po...
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/109#discussion_r140655803
--- Diff: src/protocols/vnc/user.c ---
@@ -112,6 +112,8 @@ int guac_vnc_user_leave_handler(guac_user* user) {
guac_vnc_client* vnc_client = (guac_vnc_client*) user->client->data;
+ if(!vnc_client || !user || !vnc_client->display || !vnc_client->display->cursor) return 1;
--- End diff --
To quote @mike-jumper: if is not a function. (Should be a space between the "if" and the opening parenthesis.)
:-)
Also, to be consistent with style used throughout the rest of the code, the return one should be on its own line and indented.
---
[GitHub] incubator-guacamole-server pull request #109: GUACAMOLE-388: resolve null po...
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/109#discussion_r143347204
--- Diff: src/protocols/vnc/user.c ---
@@ -112,6 +112,9 @@ int guac_vnc_user_leave_handler(guac_user* user) {
guac_vnc_client* vnc_client = (guac_vnc_client*) user->client->data;
+ if (!vnc_client || !user || !vnc_client->display || !vnc_client->display->cursor)
--- End diff --
Summarizing [my comment from earlier](https://github.com/apache/incubator-guacamole-server/pull/109#issuecomment-331722195), this check seems overly broad, and it's unlikely that all of these pointers need to be verified. We shouldn't be paranoidly checking for `NULL` every time we're about to use a pointer.
If we're going to add checks, the reason for each check should be known, and the cause of [GUACAMOLE-388](https://issues.apache.org/jira/browse/GUACAMOLE-388) needs to be determined.
---
[GitHub] incubator-guacamole-server pull request #109: GUACAMOLE-388: resolve null po...
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/109#discussion_r143374493
--- Diff: src/protocols/vnc/user.c ---
@@ -112,6 +112,9 @@ int guac_vnc_user_leave_handler(guac_user* user) {
guac_vnc_client* vnc_client = (guac_vnc_client*) user->client->data;
+ if (!vnc_client || !user || !vnc_client->display || !vnc_client->display->cursor)
--- End diff --
> I do not mind if you want to determine the root cause of GUACAMOLE-388
Excellent, as that's really the first step before attempting any sort of fix. It's not a good idea to simply cast a wide net without knowing the actual cause, and then call it fixed when the problem cannot be reproduced.
> what I wanted to do is to add check for null, it still makes sense, i.e. "check for null before dereferencing the pointer". it will help if something will change later on the caller side.
No, blanket/paranoid checks are not good practice. We shouldn't be checking for `NULL` simply as a matter of course. Things should be written purposefully, and we should know what we're passing in. Coding defensively like that is self-defeating.
---
[GitHub] incubator-guacamole-server pull request #109: GUACAMOLE-388: resolve null po...
Posted by chipitsine <gi...@git.apache.org>.
Github user chipitsine commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/109#discussion_r143347769
--- Diff: src/protocols/vnc/user.c ---
@@ -112,6 +112,9 @@ int guac_vnc_user_leave_handler(guac_user* user) {
guac_vnc_client* vnc_client = (guac_vnc_client*) user->client->data;
+ if (!vnc_client || !user || !vnc_client->display || !vnc_client->display->cursor)
--- End diff --
hello, I'm doing this in my spare time.
I do not mind if you want to determine the root cause of GUACAMOLE-388
what I wanted to do is to add check for null, it still makes sense, i.e. "check for null before dereferencing the pointer". it will help if something will change later on the caller side.
as for "overly broad", I'll make it more narrow soon
---