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:
[email protected]
