Question on thread safety

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