Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:26735
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain googlemail.com designates 64.233.182.190 as permitted sender)
DomainKey-Status: good
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=googlemail.com;
        h=received:message-id:date:from:user-agent:mime-version:to:subject:content-type:content-transfer-encoding;
        b=fM8YH7dzARXWfebDf+TSQcqrkrQFNaibUWqysEQ/ZBYNJd4bv3hT8t4cD7d7YPCNkcpji0TON+fNn2bkNcyMZWsbR+k0V9lPA0tXcdU26TaKkP2YDsg4CoyeSm1tXj3br+jIa8QW2Xo5hJLSuk0OpN+WvsoHFe5ccejGXW0JueA=
Message-ID: <456D8D30.7070405@googlemail.com>
Date: Wed, 29 Nov 2006 13:37:52 +0000
User-Agent: Thunderbird 1.5.0.8 (Windows/20061025)
MIME-Version: 1.0
To: PHP internals list <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Question on thread safety
From: andy.wharmby@googlemail.com (Andy Wharmby)

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