Question on thread safety View as Markdown

Hi All,
My first post on here but I have a come across a potential issue
with the PHP code and rather than just raise a defect thought it better
to solicit other
peoples views on the issue first.

I have been reviewing the PHP code recently in order to familiarize
myself with how it all fits together. Lately I have been focusing on
thread safety and I
have already raised a couple of defects on issues found in the code:

http://bugs.php.net/bug.php?id=39623
http://bugs.php.net/bug.php?id=39648

Other potential issues have also been identified and further defects may
follow.

However, this email relates to a question on the design of the TSRM.c
code itself. The code in/ ts_allocate_id() /which used to allocate a new
thread safe
resource id is single threaded by virtue of the mutex acquired on entry.
When a new resource is allocated, the code allocates an instance of that
resource
for each active thread as follows:

/* enlarge the arrays for the already active threads */
for (i=0; i<tsrm_tls_table_size; i++) {
    tsrm_tls_entry *p = tsrm_tls_table[i];

    while (p) {
        if (p->count < id_count) {
            int j;

            p->storage = (void *) realloc(p->storage, sizeof(void 

*)*id_count);
for (j=p->count; j<id_count; j++) {
p->storage[j] = (void *)
malloc(resource_types_table[j].size);
if (resource_types_table[j].ctor) {
resource_types_table[j].ctor(p->storage[j],
&p->storage);
}
}
p->count = id_count;
}
p = p->next;
}
}

The realloc() in the above code will potentially acquire a new memory
block, copy the contents from original block and the free the original block
(making it eligible for re-allocation) before returning to caller which
saves away the new memory blocks address in the threads/ /tsrm_tls-entry/. /

Next, looking at ts_resource_ex() which is called by a thread to get its
thread local storage for a particular resource we see:

if (!th_id) {
    /* Fast path for looking up the resources for the current
     * thread. Its used by just about every call to
     * ts_resource_ex(). This avoids the need for a mutex lock
     * and our hashtable lookup.
     */
    thread_resources = tsrm_tls_get();

    if (thread_resources) {
        TSRM_ERROR((TSRM_ERROR_LEVEL_INFO, "Fetching resource id %d 

for current thread %d",
id, (long) thread_resources->thread_id));
/* Read a specific resource from the thread's resources.
* This is called outside of a mutex, so have to be aware
about external
* changes to the structure as we read it.
*/
TSRM_SAFE_RETURN_RSRC(thread_resources->storage, id,
thread_resources->count);
}
thread_id = tsrm_thread_id();
} else {
thread_id = *th_id;
}

This is executed WITHOUT the mutex (I assume for performance reasons)
and directly accesses the same "storage" field which is modified
by ts_allocate_id(). The comment suggests someone has thought about
potential problems here but I see no code here or in
TSRM_SAFE_RETURN_RSRC that takes account of possible modifications to
the address in "storage".

My reading of the code as it currently stands is that there is a window
between the freeing of the original storage block by realloc() and the
saving away of the new memory block address in the "storage" field by
ts_allocate_id() during which time the address in "storage" is stale.
The old memory could potentially be reallocated and modified during this
window. So it is possible for a thread to access its tsrm_tls_entry
and read an old address for "storage"; potentially picking up the
address of storage which may have been reallocated to another thread and
modified. If is does so then the results are unpredictable but a
segmentation violation is one of most likely outcomes.

Further, on an architecture which has a weakly ordered memory model, e.g
PPC, there is further potential that another thread will see a stale
address even after the store into "storage" has been executed due to
absence of any memory barrier instructions in the code. If all access to
"storage" were within a mutex then this would not be an issue as the
mutex enter/release provide the necessary memory synchronization but
as ts_resource_ex() accesses the memory outside of a mutex their is no
guarantee another thread calling ts_resource_ex() will see the result
of the store.

Now having said all that I do not believe given the current usage of
ts_allocate_id() that this will cause an issue. The reason being that a
quick
scan of the code reveals that ts_allocate_id() is only called during PHP
initialization and extension initialization (MINIT) when the code is
effectively single threaded anyway so no thread will see any stale
address in "storage". However, I see nothing in the code that would stop an
extension writer from calling ts_allocate_id() outside of MINIT, e.g in
request initialization (RINIT). If this did happen then problems are
inevitable in ZTS enabled builds and the code should be fixed before it
causes issues which could be difficult to diagnose.

So finally to my question. Is it the intention of TSRMc. to allow
ts_allocate_id() to be called at any time or is there an unwritten rule
that it
should only ever called during php startup ? If its the former then I
believe the code is broken as it is and should be fixed before it causes any
problems. I am happy to investigate possible solutions. If its the
latter then I believe the rule needs to be policed by code and any
attempt to call
ts_allocate_id() after startup failed gracefully.

I myself see no reason why extension writers should be restricted from
calling ts_allocate_id() outside PHP startup so believe the code needs to be
fixed but I would appreciate the views of others with more experience
into the workings of the code on how this potential problem should be
addressed before I spend time working on any possible fix.

Regards,
Andy

Andy Wharmby
IBM United Kingdom Limited