php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #29915 [PATCH] allocate_new_resource() dangling pointer
Submitted: 2004-08-31 15:30 UTC Modified: 2005-03-20 10:05 UTC
From: test dot 007 at seznam dot cz Assigned:
Status: Closed Package: Reproducible crash
PHP Version: 4CVS, 5CVS (2005-03-09) OS: *
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: test dot 007 at seznam dot cz
New email:
PHP Version: OS:

 

 [2004-08-31 15:30 UTC] test dot 007 at seznam dot cz
Description:
------------
There's a dangling pointer in TSRM.c: allocate_new_resource(),  the last command.
After 
tsrm_mutex_unlock(tsmm_mutex);
the (*thread_resources_ptr) is invalid if you're really unlucky.

Fix:
diff ..\php-4.3.8.orig\TSRM\TSRM.c TSRM\TSRM.c
258a259
>     tsrm_tls_entry *new_resource;
261a263
>     new_resource = (*thread_resources_ptr);
291c293
<		tsrm_new_thread_end_handler(thread_id, &((*thread_resources_ptr)->storage));
---
>		tsrm_new_thread_end_handler(thread_id, &((new_resource)->storage));

Long description:
I believe there's a bug even in latest PHP 5, which crashes more probably in debug build, see TSRM/tsrm.c:
ts_resource_ex() calls while owning a mutex:
allocate_new_resource(&thread_resources->next, thread_id);
That should add a new tsrm_tls_entry* thread_resources to the end of the linked list.
allocate_new_resource(tsrm_tls_entry **thread_resources_ptr, THREAD_T thread_id) adds a new linked entry (i.e. sets the list tail's next)
(*thread_resources_ptr) = (tsrm_tls_entry *) malloc(sizeof(tsrm_tls_entry));
unlock the mutex;
tsrm_new_thread_end_handler(thread_id, &((*thread_resources_ptr)->storage)); /* 2nd arg equals to (*thread_resources_ptr) */
If another thread, by accident exactly the thread which has been the last one in the linked list before the allocate_new_resource() called malloc(), and whose "next" entry was set there, calls ts_free_thread(), it free()s the thread_resources of ts_resource_ex() -- 
in debug build, the (thread_resources->next) vulgo (*thread_resources_ptr) is set to 0xfeeefeee, and the call tsrm_new_thread_end_handler(0xfeeefeee) crashes; 
in release build, the memory can be used by another malloc() or decommited, but probably, in the very short period this dangling pointer is used, it's not overwritten, thus the code executes almost always without crash. 

The solution is to store (*thread_resources_ptr) on stack anytimes between malloc() and unlock.

Reproduce code:
---------------
You need to run multiple threads and call ts_free_thread() from them regularly (is it correct?). 
You need the thread hash table to be too small so that hash collisions occur. To crash you need the tail of a linked list to call ts_free_thread() at the same time a new tail is added.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2005-02-10 00:24 UTC] tony2001@php.net
Please try using this CVS snapshot:

  http://snaps.php.net/php5-STABLE-latest.tar.gz
 
For Windows:
 
  http://snaps.php.net/win32/php5.0-win32-latest.zip

Please provide a short but complete reproduce script if you still expirience this issue.
Also, I don't quite understand: you're talking about 5.0.x-CVS, but the patch you provided is against 4.3.x-CVS.
 [2005-02-16 16:48 UTC] test dot 007 at seznam dot cz
Hello Tony,

I found the bug in 4.3.8, but I've checked sources of PHP5 (a link you provided), and the bug is still present in both 4.x and 5.x.

The bug only occurs if there are more PHP threads running, and you have a bad luck. It is unrelated to a PHP script, it can crash with any script(s) => I can't provide you with a particular "crashing" or even "crashes here" script.

To sum up the problem, allocate_new_resource() uses a resource a microsecond after it has unlocked a mutex. If another thread removes the resource during the microsecond, crash.

tsrm_mutex_unlock(tsmm_mutex);
if (tsrm_new_thread_end_handler) {
		tsrm_new_thread_end_handler(thread_id, &((*thread_resources_ptr)->storage));

My patch is obviously harmless, and, believe me, it helps :-)
 [2005-03-07 21:35 UTC] sniper@php.net
Please provide any patches in unified diff format (diff -u)
and also, put them online somewhere where we can download them. 

Can you also give some short example script which illustrates the problem this patch supposedly fixes?

 [2005-03-08 15:29 UTC] test dot 007 at seznam dot cz
--- TSRM/TSRM.c	2004-05-23 19:05:10.000000000 +0200
+++ TSRM/TSRM.c	2005-03-08 14:36:33.250000000 +0100
@@ -260,9 +260,11 @@
 static void allocate_new_resource(tsrm_tls_entry **thread_resources_ptr, THREAD_T thread_id)
 {
 	int i;
+	tsrm_tls_entry *new_resource;
 
 	TSRM_ERROR((TSRM_ERROR_LEVEL_CORE, "Creating data structures for thread %x", thread_id));
 	(*thread_resources_ptr) = (tsrm_tls_entry *) malloc(sizeof(tsrm_tls_entry));
+	new_resource = (*thread_resources_ptr);
 	(*thread_resources_ptr)->storage = (void **) malloc(sizeof(void *)*id_count);
 	(*thread_resources_ptr)->count = id_count;
 	(*thread_resources_ptr)->thread_id = thread_id;
@@ -299,7 +301,7 @@
 	tsrm_mutex_unlock(tsmm_mutex);
 
 	if (tsrm_new_thread_end_handler) {
-		tsrm_new_thread_end_handler(thread_id, &((*thread_resources_ptr)->storage));
+		tsrm_new_thread_end_handler(thread_id, &(new_resource->storage));
 	}
 }
  
http://test-007.webpark.cz/php29915.u.patch

As I already explained, it may crash with any scripts, it only needs many concurrent threads, thus I can't give you a good example script.
 [2005-03-19 22:38 UTC] sniper@php.net
Again a patch, can you review it or do I blindly commit it?

 [2005-03-19 23:52 UTC] moriyoshi@php.net
-       tsrm_mutex_unlock(tsmm_mutex);
 
        if (tsrm_new_thread_end_handler) {
                tsrm_new_thread_end_handler(thread_id,
&((*thread_resources_ptr)->storage));
        }
+       tsrm_mutex_unlock(tsmm_mutex);
 }

IMO simply putting the mutex release point ahead would 
make more sense..


 [2005-03-20 10:05 UTC] zeev@php.net
Fixed in CVS (picked Moriyoshi's approach to be on the safe side)
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Mon Jan 27 12:01:24 2020 UTC