[GitHub] [guacamole-server] trengri opened a new pull request #272: GUACAMOLE-1053: Set to NULL after free

[GitBox <gi...@apache.org>]

trengri opened a new pull request #272:
URL: https://github.com/apache/guacamole-server/pull/272


   It will make a segfault in GUACAMOLE-1053 much less likely, but race condition between a client and user input threads remains.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] trengri commented on pull request #272: GUACAMOLE-1053: Set to NULL after free

[GitBox <gi...@apache.org>]

trengri commented on pull request #272:
URL: https://github.com/apache/guacamole-server/pull/272#issuecomment-619562726


   > I'd rather eliminate the race condition, not just make it "much less likely"
   
   I totally agree that race condition must be eliminated. However, I'm not proposing this patch _instead_ of eliminating it. This is rather a first step while working on this issue.
    
   > I'm curious if manually setting an item `null` after `free`ing it is really necessary?  It seems like something the `free` process would already do...
   
   free() just releases a memory block from a heap and does not touch an original pointer. Setting to NULL after free() is a good practice because it allows you to determine whether the memory is allocated or not.
   
   For example, see https://github.com/apache/guacamole-server/blob/9c37fc56171d55b8fa838949d7fd40db85506857/src/protocols/rdp/input.c#L130-L136
   
   As you can see, it tests **rdp_client->keyboard** for a NULL value and calls **guac_rdp_keyboard_update_keysym** (the function where the segfault occurred) only if it is not NULL. If it is set to NULL, it will not call **guac_rdp_keyboard_update_keysym**() and segfault will not happen.
   
   If you free the memory, but don't assign a pointer a NULL value, you will then have no chance to check if this memory is allocated or not. If the code above sees any non-NULL pointer, it will think it is allocated and call **guac_rdp_keyboard_update_keysym**() which will result in segfault. 
   
   When I was talking about a race condition, I meant the following sequence of events:
   
   1. One thread (i.e. a user input thread) tests a pointer for a NULL value, sees it is not NULL and starts accessing the memory using this pointer.
   2. The thread timeslice expires and the kernel preempts it, giving the control to the client thread. RDP client disconnects (for whatever reason) and frees the memory.
   3. When the original user input thread continues to run, it will find the memory freed.
   
   As you can see, this scenario is much less likely than the original issue, but still possible. I can try fixing it, but it will be much complex patch than this trivial one.
   
   I have submitted this PR because I think that if you test a pointer for a NULL value anywhere in the code, you should assign it NULL after the memory is freed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [guacamole-server] necouchman commented on pull request #272: GUACAMOLE-1053: Set to NULL after free

[GitBox <gi...@apache.org>]

necouchman commented on pull request #272:
URL: https://github.com/apache/guacamole-server/pull/272#issuecomment-619547715


   Two issues I have with this PR:
   * I'd rather eliminate the race condition, not just make it "much less likely"
   * I'm curious if manually setting an item `null` after `free`ing it is really necessary?  It seems like something the `free` process would already do, or at least to an extent that wouldn't lead to a race condition.  Seems like solving the race condition is more preferable to putting a work-around in place that _might_ reduce its likelihood.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org