[Python-checkins] CVS: python/dist/src/Objects obmalloc.c,2.18,2.19

[Tim Peters]

Update of /cvsroot/python/python/dist/src/Objects
In directory usw-pr-cvs1:/tmp/cvs-serv13490/python/Objects

Modified Files:
	obmalloc.c 
Log Message:
It's once again thought safe to call the pymalloc free/realloc with an
address obtained from system malloc/realloc without holding the GIL.

When the vector of arena base addresses has to grow, the old vector is
deliberately leaked.  This makes "stale" x-thread references safe.
arenas and narenas are also declared volatile, and changed in an order
that prevents a thread from picking up a value of narenas too large
for the value of arenas it sees.

Added more asserts.

Fixed an old inaccurate comment.

Added a comment explaining why it's safe to call pymalloc free/realloc
with an address obtained from system malloc/realloc even when arenas is
still NULL (this is obscure, since the ADDRESS_IN_RANGE macro
appears <wink> to index into arenas).


Index: obmalloc.c
===================================================================
RCS file: /cvsroot/python/python/dist/src/Objects/obmalloc.c,v
retrieving revision 2.18
retrieving revision 2.19
diff -C2 -d -r2.18 -r2.19
*** obmalloc.c	30 Mar 2002 10:42:09 -0000	2.18
--- obmalloc.c	30 Mar 2002 21:36:04 -0000	2.19
***************
*** 172,176 ****
  /*
   * Size of the pools used for small blocks. Should be a power of 2,
!  * between 1K and SYSTEM_PAGE_SIZE, that is: 1k, 2k, 4k, eventually 8k.
   */
  #define POOL_SIZE		SYSTEM_PAGE_SIZE	/* must be 2^N */
--- 172,176 ----
  /*
   * Size of the pools used for small blocks. Should be a power of 2,
!  * between 1K and SYSTEM_PAGE_SIZE, that is: 1k, 2k, 4k.
   */
  #define POOL_SIZE		SYSTEM_PAGE_SIZE	/* must be 2^N */
***************
*** 309,314 ****
   * to the OS.
   */
! static uptr *arenas = NULL;
! static uint narenas = 0;
  static uint maxarenas = 0;
  
--- 309,314 ----
   * to the OS.
   */
! static uptr *volatile arenas = NULL;	/* the pointer itself is volatile */
! static volatile uint narenas = 0;
  static uint maxarenas = 0;
  
***************
*** 355,358 ****
--- 355,359 ----
  	arenabase = bp;
  	nfreepools = ARENA_SIZE / POOL_SIZE;
+ 	assert(POOL_SIZE * nfreepools == ARENA_SIZE);
  	excess = (uint)bp & POOL_SIZE_MASK;
  	if (excess != 0) {
***************
*** 363,375 ****
  	/* Make room for a new entry in the arenas vector. */
  	if (arenas == NULL) {
  		arenas = (uptr *)PyMem_MALLOC(16 * sizeof(*arenas));
  		if (arenas == NULL)
  			goto error;
  		maxarenas = 16;
- 		narenas = 0;
  	}
  	else if (narenas == maxarenas) {
  		/* Grow arenas.  Don't use realloc:  if this fails, we
  		 * don't want to lose the base addresses we already have.
  		 * Exceedingly subtle:  Someone may be calling the pymalloc
  		 * free via PyMem_{DEL, Del, FREE, Free} without holding the
--- 364,377 ----
  	/* Make room for a new entry in the arenas vector. */
  	if (arenas == NULL) {
+ 		assert(narenas == 0 && maxarenas == 0);
  		arenas = (uptr *)PyMem_MALLOC(16 * sizeof(*arenas));
  		if (arenas == NULL)
  			goto error;
  		maxarenas = 16;
  	}
  	else if (narenas == maxarenas) {
  		/* Grow arenas.  Don't use realloc:  if this fails, we
  		 * don't want to lose the base addresses we already have.
+ 		 *
  		 * Exceedingly subtle:  Someone may be calling the pymalloc
  		 * free via PyMem_{DEL, Del, FREE, Free} without holding the
***************
*** 393,407 ****
  		 * make a supposed-to-fail case succeed by mistake (it could
  		 * only make a supposed-to-succeed case fail by mistake).
  		 * Read the above 50 times before changing anything in this
  		 * block.
- 		 * XXX Fudge.  This is still vulnerable:  there's nothing
- 		 * XXX to stop the bad-guy thread from picking up the
- 		 * XXX current value of arenas, but not indexing off of it
- 		 * XXX until after the PyMem_FREE(oldarenas) below completes.
  		 */
- 		uptr *oldarenas;
  		uptr *p;
! 		uint newmax = maxarenas + (maxarenas >> 1);
! 
  		if (newmax <= maxarenas)	/* overflow */
  			goto error;
--- 395,415 ----
  		 * make a supposed-to-fail case succeed by mistake (it could
  		 * only make a supposed-to-succeed case fail by mistake).
+ 		 *
+ 		 * In addition, without a lock we can't know for sure when
+ 		 * an old vector is no longer referenced, so we simply let
+ 		 * old vectors leak.
+ 		 *
+ 		 * And on top of that, since narenas and arenas can't be
+ 		 * changed as-a-pair atomically without a lock, we're also
+ 		 * careful to declare them volatile and ensure that we change
+ 		 * arenas first.  This prevents another thread from picking
+ 		 * up an narenas value too large for the arenas value it
+ 		 * reads up (arenas never shrinks).
+ 		 *
  		 * Read the above 50 times before changing anything in this
  		 * block.
  		 */
  		uptr *p;
! 		uint newmax = maxarenas << 1;
  		if (newmax <= maxarenas)	/* overflow */
  			goto error;
***************
*** 410,416 ****
  			goto error;
  		memcpy(p, arenas, narenas * sizeof(*arenas));
! 		oldarenas = arenas;
! 		arenas = p;
! 		PyMem_FREE(oldarenas);
  		maxarenas = newmax;
  	}
--- 418,422 ----
  			goto error;
  		memcpy(p, arenas, narenas * sizeof(*arenas));
! 		arenas = p;	/* old arenas deliberately leaked */
  		maxarenas = newmax;
  	}
***************
*** 432,435 ****
--- 438,442 ----
   * pymalloc.  I must be the index into arenas that the address claims
   * to come from.
+  *
   * Tricky:  Letting B be the arena base address in arenas[I], P belongs to the
   * arena if and only if
***************
*** 438,441 ****
--- 445,454 ----
   *	0 <= P-B < ARENA_SIZE
   * By using unsigned arithmetic, the "0 <=" half of the test can be skipped.
+  *
+  * Obscure:  A PyMem "free memory" function can call the pymalloc free or
+  * realloc before the first arena has been allocated.  arenas is still
+  * NULL in that case.  We're relying on that narenas is also 0 in that case,
+  * so the (I) < narenas must be false, saving us from trying to index into
+  * a NULL arenas.
   */
  #define ADDRESS_IN_RANGE(P, I) \