Mailing List Archive

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) \