Races in guacd

[Grigory Trenin <gt...@gmail.com>]

Hi Guacamole developers,

I think I have found a couple of races in gaucd (this is aside from
GUACAMOLE-1053, which is a third one).

1) libguac starts a user input thread and detaches from it. The problem is
that it does not wait for this thread to terminate before calling
client->free_handler. Doing so opens the road to race conditions if
client->free_handler is freeing a resource (eg: mutex) that is used in user
input handlers.

2) You are using fork() in a multithreaded program. If you are doing that,
a caution must be taken not to call fork() at the same time when another
thread is calling any libc function that uses locks, such as malloc() or
free(). Better to avoid such situation at all, eg: call fork() as early as
possible (when no threads are created yet), and then create all required
threads (but it requires some architecture changes to guacd).

What do you think?

Kind regards,
Grigory Trenin

Re: Races in guacd

[Grigory Trenin <gt...@gmail.com>]

> When the client is being cleaned up, ensuring that the input thread (the
> thread handling inbound Guacamole instructions) is terminated first makes
> sense. I think this is worth doing, and then documenting the guarantee that
> no other handlers will be in-progress or invoked after the free_handler is
> invoked.
>
>
Thanks for confirming. I will raise a JIRA ticket and create a pull request
for it.

Do you have a specific example of a place where such a race is possible
> within guacd?
>

Suppose guacd is running for some time, and there are 50 concurrent RDP
sessions now. A new user joins and fork() is called in guacd_create_proc().
Since there are 50 concurrent users already, the process that is being
forked contains a plenty of i/o connection threads (2 threads per user:
read thread and write thread). So for 50 users there are 100 threads in
this process. And each of this thread can call free() at arbitrary time
when the connection is closed.
The problem can heppen if free() in any of these threads is called at the
same time when fork() is called for the new user.


Kind regards,
Grigory  Trenin

Re: Races in guacd

[Mike Jumper <mj...@apache.org>]

On Tue, May 12, 2020 at 5:31 AM Grigory Trenin <gt...@gmail.com> wrote:

> Hi Guacamole developers,
>
> I think I have found a couple of races in gaucd (this is aside from
> GUACAMOLE-1053, which is a third one).
>
> 1) libguac starts a user input thread and detaches from it. The problem is
> that it does not wait for this thread to terminate before calling
> client->free_handler. Doing so opens the road to race conditions if
> client->free_handler is freeing a resource (eg: mutex) that is used in user
> input handlers.
>

When the client is being cleaned up, ensuring that the input thread (the
thread handling inbound Guacamole instructions) is terminated first makes
sense. I think this is worth doing, and then documenting the guarantee that
no other handlers will be in-progress or invoked after the free_handler is
invoked.


> 2) You are using fork() in a multithreaded program. If you are doing that,
> a caution must be taken not to call fork() at the same time when another
> thread is calling any libc function that uses locks, such as malloc() or
> free(). Better to avoid such situation at all, eg: call fork() as early as
> possible (when no threads are created yet), and then create all required
> threads (but it requires some architecture changes to guacd).
>

Do you have a specific example of a place where such a race is possible
within guacd?

I recall considering this when screen sharing support was being developed,
and attempting to take this into account when using fork() alongside code
that needed to be threaded, but I may simply be wrong.

- Mike