Porting to system without pthread_mutexattr_setpshare

[Steve Williams <st...@williamsitconsulting.com>]

Hi,

I apologize if this is not an appropriate forum to be posting this.

I am looking into guacamole's use of   pthread_mutexattr_setpshare.

I am trying to port/compile guacamole 1.1.0 on OpenBSD.  The only 
(significant) errors I am getting pertain to the use of 
pthread_mutexattr_setpshared(foo, PTHREAD_PROCESS_SHARED).

If I comment out the 4 lines of code invoking 
pthread_mutexattr_setpshare in the source tree, I can it to compile with 
gcc on OpenBSD 6.6 (GENERIC.MP) )

I am an experienced C programmer, but I've never looked in threading in 
C before so it has required quite a bit of reading.  I haven't been able 
to find a comprehensive "architecture" document for pthreads, just 
various man pages, some with sample code.

For example:
https://linux.die.net/man/3/pthread_mutexattr_init

In the above documentation, it states:

    ...the possibility that an application may allocate the
    synchronization objects from this section in memory that is accessed
    by multiple processes (and therefore, by threads of multiple 
processes).

It appears that the purpose of the guacamole's 
pthread_mutexattr_setpshared(foo, PTHREAD_PROCESS_SHARED) is to permit 
multiple threaded processes to access the resource protected by the MUTEX.

However, from my reading of the guacamole code, the MUTEX is only 
protecting malloc'd memory, which as far as I know, isn't a resource 
that can be accessed by multiple processes.

It is my newby (to pthreads) interpretation of the code that in all 4 
cases, the code is malloc'ing memory that is being protected by the 
MUTEX.  For example:

src/libguac/pool.c:
...
guac_pool* guac_pool_alloc(int size) {

     pthread_mutexattr_t lock_attributes;
     guac_pool* pool = malloc(sizeof(guac_pool));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Allocate memory that will only be accessible by this process

     /* If unable to allocate, just return NULL. */
     if (pool == NULL)
         return NULL;

     /* Initialize empty pool */
     pool->min_size = size;
     pool->active = 0;
     pool->__next_value = 0;
     pool->__head = NULL;
     pool->__tail = NULL;

     /* Init lock */
     pthread_mutexattr_init(&lock_attributes);
     pthread_mutexattr_setpshared(&lock_attributes, 
PTHREAD_PROCESS_SHARED);
     pthread_mutex_init(&(pool->__lock), &lock_attributes);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This codes saves the MUTEX in the locally allocated memory

No other process can find this MUTEX in malloc'd memory, so setting it 
to PTHREAD_PROCESS_SHARED seems totally irrelevant.

If so, I *think* it's OK to just comment out the 
pthread_mutexattr_setpshared code as OpenBSD's pthread MUTEX locking 
implementation will work fine within a process, just not multiple 
processes.

Is this a correct assessment of the code?

Thanks,
Steve W.

Re: Porting to system without pthread_mutexattr_setpshare

[Nick Couchman <vn...@apache.org>]

On Sat, Apr 18, 2020 at 3:29 AM Steve Williams <
steve@williamsitconsulting.com> wrote:

> Hi,
>
> I apologize if this is not an appropriate forum to be posting this.
>
> I am looking into guacamole's use of   pthread_mutexattr_setpshare.
>
> I am trying to port/compile guacamole 1.1.0 on OpenBSD.  The only
> (significant) errors I am getting pertain to the use of
> pthread_mutexattr_setpshared(foo, PTHREAD_PROCESS_SHARED).
>
> If I comment out the 4 lines of code invoking
> pthread_mutexattr_setpshare in the source tree, I can it to compile with
> gcc on OpenBSD 6.6 (GENERIC.MP) )
>
> I am an experienced C programmer, but I've never looked in threading in
> C before so it has required quite a bit of reading.  I haven't been able
> to find a comprehensive "architecture" document for pthreads, just
> various man pages, some with sample code.
>
> For example:
> https://linux.die.net/man/3/pthread_mutexattr_init
>
> In the above documentation, it states:
>
>     ...the possibility that an application may allocate the
>     synchronization objects from this section in memory that is accessed
>     by multiple processes (and therefore, by threads of multiple
> processes).
>
> It appears that the purpose of the guacamole's
> pthread_mutexattr_setpshared(foo, PTHREAD_PROCESS_SHARED) is to permit
> multiple threaded processes to access the resource protected by the MUTEX.
>
> However, from my reading of the guacamole code, the MUTEX is only
> protecting malloc'd memory, which as far as I know, isn't a resource
> that can be accessed by multiple processes.
>
> It is my newby (to pthreads) interpretation of the code that in all 4
> cases, the code is malloc'ing memory that is being protected by the
> MUTEX.  For example:
>
> src/libguac/pool.c:
> ...
> guac_pool* guac_pool_alloc(int size) {
>
>      pthread_mutexattr_t lock_attributes;
>      guac_pool* pool = malloc(sizeof(guac_pool));
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Allocate memory that will only be accessible by this process
>
>      /* If unable to allocate, just return NULL. */
>      if (pool == NULL)
>          return NULL;
>
>      /* Initialize empty pool */
>      pool->min_size = size;
>      pool->active = 0;
>      pool->__next_value = 0;
>      pool->__head = NULL;
>      pool->__tail = NULL;
>
>      /* Init lock */
>      pthread_mutexattr_init(&lock_attributes);
>      pthread_mutexattr_setpshared(&lock_attributes,
> PTHREAD_PROCESS_SHARED);
>      pthread_mutex_init(&(pool->__lock), &lock_attributes);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This codes saves the MUTEX in the locally allocated memory
>
> No other process can find this MUTEX in malloc'd memory, so setting it
> to PTHREAD_PROCESS_SHARED seems totally irrelevant.
>
> If so, I *think* it's OK to just comment out the
> pthread_mutexattr_setpshared code as OpenBSD's pthread MUTEX locking
> implementation will work fine within a process, just not multiple
> processes.
>
>
So, the only thing that I wonder that this might impact would be connection
sharing.  I don't know for sure that this is the case - it may be that the
connection sharing is done in such a way that doesn't require this, but
based on my knowledge of how Guacamole works and the connection sharing
involving someone initiating another connection to an existing running
connection (=process) in guacd, it seems like this could break that or at
the very least cause some undesirable behavior.

I could be off on that - I'd be interested to see what others have to say
about that - it's just my initial thought without diving into the code, and
based on my fairly limited knowledge of pthreads.

-Nick