# Question on thread safety - Thread: https://externals.io/message/26735 - Original archive: https://news-web.php.net/php.internals/26735 - Started by Andy Wharmby on 2006-11-29T13:37:52+00:00 - Messages: 3 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #26735 — Andy Wharmby — 2006-11-29T13:37:52+00:00 URL: https://externals.io/message/26735#26735 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; icount < id_count) { int j; p->storage = (void *) realloc(p->storage, sizeof(void *)*id_count); for (j=p->count; jstorage[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 --- ## #26738 — Stanislav Malyshev — 2006-11-29T22:16:02+00:00 URL: https://externals.io/message/26735#26738 · In reply to: #26735 > 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 I think it gets called only on startup. I also think it was the intent, though there is no safeguard as far as I can see against calling it in run-time, but no module does it and it doesn't make sense to do it in other place than startup. > I myself see no reason why extension writers should be restricted from > calling ts_allocate_id() outside PHP startup so believe the code needs Well, the reason is that if you want to use TSRM globals, you have to allocate ID before you do basically anything with them. Startup is a good place for that. If you don't need globals, then you should not call it at all. The situation where in the mid-run you suddenly remember you need globals seems quite unrealistic to me. Of course, if you can describe scenario when you would really need it in mid-run or it would make sense to allocate ID in mid-run, then I guess this should be fixed or at least safeguarded. -- Stanislav Malyshev, Zend Products Engineer stas@zend.com http://www.zend.com/ --- ## #26741 — Andy Wharmby — 2006-11-30T14:49:06+00:00 URL: https://externals.io/message/26735#26741 · In reply to: #26738 Stanislav Malyshev wrote: > > […] > > I think it gets called only on startup. I also think it was the > intent, though there is no safeguard as far as I can see against > calling it in run-time, but no module does it and it doesn't make > sense to do it in other place than startup. > > > […] > > Well, the reason is that if you want to use TSRM globals, you have to > allocate ID before you do basically anything with them. Startup is a > good place for that. If you don't need globals, then you should not > call it at all. The situation where in the mid-run you suddenly > remember you need globals seems quite unrealistic to me. Of course, if > you can describe scenario when you would really need it in mid-run or > it would make sense to allocate ID in mid-run, then I guess this > should be fixed or at least safeguarded. I did not have any particular scenario in mind here as I too could not come up with a sensible reason to allow calls to ts_allocate_id() outside initialization. Initially I thought of the case of a user script loading an extension using `dl()` but soon found out this is policed and not allowed if ZTS is enabled. The reason I assumed that ts_allocate_id() was designed to be called at any time was the fact that the code is wrapped in a mutex, which lead to my concerns about ts_resource_ex(). If ts_allocate_id() is not designed to be called outside startup then my concerns about ts_resource_ex() are unfounded. However, the mutex acquire and release calls in ts_allocate_id() are therefore unnecessary and should be removed. However, I do believe this restriction should be policed to fail any calls outside startup. I see nothing in the code to stop a extension writer calling ts_allocate_id at runtime. Why would anyone do this ? So TSRM global storage is only allocated when an extension is actually needed rather than when a new thread is started. What if a user has an extension that only gets called in some exceptional circumstance and they design it so the ts_allocate_id() call is made on the first call to the extension to save storage being allocated for threads until its needed. Unlikely I know but sometimes users do what you least expect so the code should protect them wherever possible from them doing something which will: (a) cause them grief, and (b) probably lead them into raising a bogus defect and waste someones time diagnosing the problem A simple check in the code could prevent all this. There are further routines in TSRM.c which also acquire the tsmm_mutex were the reason for this is not clear given current usage: * ts_free_id(). Only called at MSHUTDOWN when single threaded so no apparent need for mutex. Again easily policed to ensure calls outside startup/shutdown not allowed. * ts_free_worker_threads(): Only called by php_module_shutdown when single threaded so no need for mutex. * ts_resource_ex: Needs mutex whilst it updates tsrm_tls_table for a new thread but it looks like it keeps mutex longer than it need do. By reworking this routine and allocate_new_resource() I believe the time the mutex is held could be reduced. I am happy to work on a patch for all this and will raise a defect with patch when I have something worthy of consideration..