Re: [PATCH,RFA] Ensure check_may_shrink_heap always returns the final value

[Alexandre Oliva <aoliva at redhat dot com>]

On Sep 18, 2013, Andreas Schwab <schwab@suse.de> wrote:

> Alexandre Oliva <aoliva@redhat.com> writes:
>> +++ b/sysdeps/unix/sysv/linux/malloc-sysdep.h
>> -  static int may_shrink_heap = -1;
>> +  static int global_may_shrink_heap = -1;
>> +  int may_shrink_heap = global_may_shrink_heap;

> I don't think this will suffice, the compiler may still fall back to
> reload the value from the static.

I guess I wasn't clear as to what the problem was.

The variable is unconditionally modified early in the function (see
below), so that would have taken care of dissociating it from the global
variable, if that was the problem.

But the problem is not in what we read: it's ok-ish if we read an
outdated value, because the initialization is idempotent.

The problem was *writing* a temporary value to the global variable; with
the change, we write to an auto variable instead, which can't possibly
cause other threads to base on a non-final value their decision on
whether or not to run the initialization code.

  static int may_shrink_heap = -1;

  if (__builtin_expect (may_shrink_heap >= 0, 1))
    return may_shrink_heap;

  may_shrink_heap = __libc_enable_secure;

  [...]

We can describe the entire function in the following 6 steps:

1. read may_shrink_heap
2. conditionally read and return may_shrink_heap
3. write temp value to may_shrink_heap
4. read may_shrink_heap
5. conditionally write final value to may_shrink_heap
6. read and return may_shrink_heap

The problem I'm addressing is that of one thread, say T1, executing
T1.3, and then another thread, say T2, running T2.1. and T2.2.,
returning the temp value written in T1.3. instead of either (a) waiting
for T1 to complete the initialization or (b) performing the
initialization itself.

With my change, step 3 writes to an auto variable, so other threads will
not see the temp value, so they will not return at 2. unless some other
thread has already completed the initialization; it will instead (b)
perform the initialization itself, and then, if some other thread was
already performing the initialization too, both will complete and come
to the same value, writing and overwriting it in the global variable,
which will eventually be used by all threads.

I.e., instead of 3 possibilities:

i. thread sees the final value and returns it
ii. thread sees an uninitialized value and performs the initialization
iii. thread sees the temporary initization value and returns it

by using the automatic variable I remove case iii, thereby fixing the
problem.


Now, it is true that a data race remains: in the absence of
synchronization, threads may observe outdated values, and we may even
have concurrent writes.  However, since observing an outdated value
results in local performance of the initialization, and concurrent
writes will write the same value, the races that remain are harmless.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer