Re: [Bug-gnulib] addition: wait-process.h, wait-process.c, 2nd round

[Paul Eggert]

Bruno Haible <address@hidden> writes:

> Comments?

I haven't had time to read the code carefully, but here are a few
random comments anyway.

> typedef struct
> {
>   volatile sig_atomic_t used;
>   volatile pid_t child;
> }
> slaves_entry_t;

"slaves_entry_t" violates the namespace rules for applications, which
aren't allowed to define anything ending in "_t" if they include the
standard include files.  Perhaps it should just be called "slaves_entry".

I'm a bit dubious about the semantics, portability, and
understandability of structs whose members are volatile.  I'd be more
comfortable if the typedef didn't mention 'volatile'; instead, you can
have the corresponding vars be 'volatile'.  E.g.:

static slaves_entry_t volatile static_slaves[32];
static slaves_entry_t volatile * volatile slaves = static_slaves;

and similarly for the other slaves_entry_t variables.

> static size_t slaves_allocated = SIZEOF (static_slaves);

Hmm, why doesn't this need to be volatile?

>       slaves_entry_t *new_slaves =
>       malloc (new_slaves_allocated * sizeof (slaves_entry_t));

That multiplication can overflow, which would cause serious memory buffer
overruns later on.  You need to add something like this:

    if (SIZE_MAX / sizeof (slaves_entry_t) < new_slaves_allocated)
      xalloc_die ();

> /* Unregister a child from the list of slave subprocesses.  */
> static inline void
> unregister_slave_subprocess (pid_t child)
> {
>   /* The easiest way to remove an entry from a list that can be used by
>      an asynchronous signal handler is just to mark it as unused.  For this,
>      we rely on sig_atomic_t.  */
>   slaves_entry_t *s = slaves;
>   slaves_entry_t *s_end = s + slaves_count;
> 
>   for (; s < s_end; s++)
>     if (s->used && s->child == child)
>       s->used = 0;
> }

Isn't there a race here?  Just before 's->used = 0' is executed, a
process could have died and another process could have been
registered.  Or perhaps there are some constraints on when the
functions can be called (e.g., you can't call these functions from a
signal handler?).