On 12/02/10 13:47, Ralf Wildenhues wrote:
> * error_at_line.c ...
> IMVHO it makes sense to at least document the requirement for the caller
> here.
Yes. This is also a restriction for the glibc implementation, no?
So it's fine if gnulib has the same restriction.
> * in getloadavg.c, getloadavg_initialized should probably be a
> sig_atomic_t not a bool (no idea whether this can ever be a problem in
> practice[1]).
In general sig_atomic_t does not guarantee atomic access with C99, any
more than bool does, across threads. (C0x may be different; I haven't
been following that.) In practice, loading and storing words tends to
be atomic across threads, if the instructions are not optimized away;
'volatile' may be needed to prevent that, but I don't offhand see why
'volatile' would be required here, even with link-time optimization.
That being said, there are race conditions there, even if one assumes
sig_atomic_t access is atomic across threads, since multiple threads
could be initializing the buffers simultaneously. Fixing this won't
be trivial. It may well be better to document getloadavg as not being
thread-safe, for now.
> * register_close_hook is not thread-safe.
Yes, worth documenting.
> I'm sure a number of other issues are lurking there. The increased use
> of LTO (link-time optimization) will surely over time expose more of
> these issues (also things like missing volatile) due to the compiler
> being able to optimize much more aggressively.
Yes and yes!
> * Unless STACK_DIRECTION is defined, gnulib/lib/alloca.c sets and uses
> STACK_DIR and find_stack_direction:addr in the first alloca call. When
> that first call is from threads, and racing with another one, the value
> for STACK_DIRECTION may be computed wrongly, and the code may corrupt
> the stack.
This should be fixable by making 'addr' auto instead of static, no?
Something like the following (untested) patch. There is another race
if loading and storing into an 'int' is not atomic, which I suppose we
could document as an assumption (a safe one, I hope, as noted above).
diff --git a/lib/alloca.c b/lib/alloca.c
index b652765..2f5e27e 100644
--- a/lib/alloca.c
+++ b/lib/alloca.c
@@ -94,21 +94,20 @@ static int stack_dir; /* 1 or -1 once known. */
# define STACK_DIR stack_dir
static void
-find_stack_direction (void)
+find_stack_direction (char **ptr)
{
- static char *addr = NULL; /* Address of first `dummy', once known. */
auto char dummy; /* To get stack address. */
- if (addr == NULL)
+ if (*ptr == NULL)
{ /* Initial entry. */
- addr = ADDRESS_FUNCTION (dummy);
+ *ptr = ADDRESS_FUNCTION (dummy);
- find_stack_direction (); /* Recurse once. */
+ find_stack_direction (ptr); /* Recurse once. */
}
else
{
/* Second entry. */
- if (ADDRESS_FUNCTION (dummy) > addr)
+ if (ADDRESS_FUNCTION (dummy) > *ptr)
stack_dir = 1; /* Stack grew upward. */
else
stack_dir = -1; /* Stack grew downward. */
@@ -155,7 +154,10 @@ alloca (size_t size)
# if STACK_DIRECTION == 0
if (STACK_DIR == 0) /* Unknown growth direction. */
- find_stack_direction ();
+ {
+ char *addr = NULL; /* Address of first `dummy', once known. */
+ find_stack_direction (&addr);
+ }
# endif
/* Reclaim garbage, defined as all alloca'd storage that
