Fixed double-free problems in GCKit. Fixed removal from hash tables. Added enumerator helper for when you remove objects from a map while enumerating. Removed tracing statements that were identifying bugs that have now been fixed.

This commit is contained in:
theraven 2010-02-03 13:46:32 +00:00
parent 8642bf7b8d
commit 7c03c0d3f1
5 changed files with 158 additions and 37 deletions

View File

@ -6,9 +6,28 @@
#import "cycle.h"
#import "visit.h"
#import "workqueue.h"
#import "static.h"
#include <stdlib.h>
#include <stdio.h>
/**
* Pointer comparison. Needed for the hash table.
*/
static int pointer_compare(const id a, const id b)
{
return a == b;
}
static int pointer_hash(const void *obj)
{
intptr_t ptr = (intptr_t)obj;
return (ptr >> 8) | (ptr << 8);
}
#define MAP_TABLE_NAME known_object
#define MAP_TABLE_COMPARE_FUNCTION pointer_compare
#define MAP_TABLE_HASH_KEY pointer_hash
#define MAP_TABLE_HASH_VALUE pointer_hash
#include "../hash_table.h"
@interface GCObject
- (void)finalize;
@end
@ -91,6 +110,7 @@ void *GCAllocateBufferWithZone(void *zone, size_t size, BOOL scan)
void GCWeakRelease(id anObject)
{
if (!GCObjectIsDynamic(anObject)) { return; }
long count = GCDecrementWeakCount(anObject);
// If the object has been finalized and this is the last weak ref, free it.
if (count == 0 && GCColourOfObject(anObject) == GCColourOrange)
@ -100,6 +120,7 @@ void GCWeakRelease(id anObject)
}
id GCWeakRetain(id anObject)
{
if (!GCObjectIsDynamic(anObject)) { return anObject; }
// If this object has already been finalized, return nil.
if (GCColourOfObject(anObject) == GCColourOrange)
{
@ -129,6 +150,7 @@ static void releaseObjects(id object, void *context, BOOL isWeak)
*/
void GCFreeObjectUnsafe(id object)
{
if (!GCObjectIsDynamic(object)) { return; }
if (GCColourOrange != GCSetColourOfObject(object, GCColourOrange))
{
GCTracedRegion region = {object, object};
@ -136,7 +158,6 @@ void GCFreeObjectUnsafe(id object)
// finalize it.
if (!GCTestFlag(object, GCFlagNotObject))
{
fprintf(stderr, "Finalizing object %x\n", (int)(object));
GCVisitChildren(object, releaseObjects, NULL, YES);
[object finalize];
region.end += class_getInstanceSize(object->isa);
@ -150,7 +171,7 @@ void GCFreeObjectUnsafe(id object)
}
if (GCGetWeakRefCount(object) == 0)
{
fprintf(stderr, "Freeing object %x\n", (int)(object));
//fprintf(stderr, "Freeing object %x\n", (int)(object));
gc_free_with_zone(GCHeaderForObject(object)->zone, object);
}
}

12
GCKit/static.h Normal file
View File

@ -0,0 +1,12 @@
#include <dlfcn.h>
/**
* Check if an object is in one of the sections that the loader allocated. If
* so, it won't have a GCKit header so we just assume that it never needs
* collecting.
*/
static inline BOOL GCObjectIsDynamic(id obj)
{
Dl_info i;
return !dladdr(obj, &i);
}

View File

@ -82,6 +82,14 @@ int main(int argc, char **argv, char **env)
#include <stdio.h>
#include <unistd.h>
@interface NSConstantString
{
id isa;
char *c_str;
unsigned len;
}
@end
@interface SimpleObject
{
Class isa;
@ -92,11 +100,12 @@ int main(int argc, char **argv, char **env)
@implementation SimpleObject
+ (id)new
{
return GCAllocateObjectWithZone(self, NULL, 0);
id obj = GCAllocateObjectWithZone(self, NULL, 0);
return obj;
}
- (void)log
{
fprintf(stderr, "Simple object is still alive\n");
fprintf(stderr, "Simple object %x is still alive\n", (int)self);
}
- (void)finalize
{
@ -107,7 +116,6 @@ int main(int argc, char **argv, char **env)
void makeObject(void)
{
SimpleObject *foo = [SimpleObject new];
fprintf(stderr, "foo (%x) is at stack address %x\n", (int)foo, (int)&foo);
[foo log];
GCDrain(YES);
GCDrain(YES);
@ -141,7 +149,7 @@ static id *buffer;
void putObjectInBuffer(void)
{
buffer = GCAllocateBufferWithZone(NULL, sizeof(id), YES);
buffer = (id*)GCRetain((id)GCAllocateBufferWithZone(NULL, sizeof(id), YES));
buffer[0] = objc_assign_strongCast([SimpleObject new], buffer);
[*buffer log];
GCDrain(YES);

View File

@ -7,11 +7,13 @@
#include <sys/limits.h>
#include <assert.h>
#include <stdio.h>
#include <dlfcn.h>
#include "../objc/runtime.h"
#import "object.h"
#import "thread.h"
#import "trace.h"
#import "malloc.h"
#import "static.h"
/**
* Structure storing pointers that are currently being traced.
@ -506,11 +508,18 @@ void GCTraceStackSynchronous(GCThread *thr)
id ptr;
while ((ptr = unescaped_object_next(thr->unescapedObjects, &e)))
{
if (!GCTestFlag(ptr, GCFlagVisited))
id oldPtr;
// Repeat on the current enumerator spot while we are are deleting things.
do
{
unescaped_object_remove(thr->unescapedObjects, ptr);
GCFreeObject(ptr);
}
oldPtr = ptr;
if (!GCTestFlag(ptr, GCFlagVisited))
{
GCFreeObject(ptr);
unescaped_object_remove(thr->unescapedObjects, ptr);
}
} while ((oldPtr != (ptr = unescaped_object_current(thr->unescapedObjects, &e)))
&& ptr);
}
thr->scannedInGeneration = generation;
}
@ -534,15 +543,22 @@ void GCRunTracer(void)
GCTracedPointer *object;
while ((object = traced_object_next(traced_objects, &e)))
{
// If an object hasn't been visited and we have scanned everywhere
// since we cleared its visited flag, delete it. This works because
// the heap write barrier sets the visited flag.
if(!GCTestFlag(object->pointer, GCFlagVisited) &&
object->visitClearedGeneration < threadGeneration)
GCTracedPointer *oldPtr;
// Repeat on the current enumerator spot while we are are deleting things.
do
{
GCFreeObjectUnsafe(object->pointer);
traced_object_remove(traced_objects, object->pointer);
}
oldPtr = object;
// If an object hasn't been visited and we have scanned everywhere
// since we cleared its visited flag, delete it. This works
// because the heap write barrier sets the visited flag.
if (!GCTestFlag(object->pointer, GCFlagVisited) &&
object->visitClearedGeneration < threadGeneration)
{
GCFreeObjectUnsafe(object->pointer);
traced_object_remove(traced_objects, object->pointer);
}
} while (oldPtr != ((object = traced_object_current(traced_objects, &e)))
&& object);
}
}
@ -603,6 +619,10 @@ void GCAddObjectsForTracing(GCThread *thr)
for (unsigned int i=0 ; i<count ; i++)
{
id object = buffer[i];
if (!GCObjectIsDynamic(object))
{
continue;
}
// Skip objects that have a strong retain count > 0. They are
// definitely still referenced.
if (GCGetRetainCount(object) > 0)
@ -615,12 +635,20 @@ void GCAddObjectsForTracing(GCThread *thr)
// if it is, but do update its generation. It was seen by
// something in this thread, so it might still be on the stack
// here, or have been moved to the heap.
GCTracedPointer obj = {object, 0, 0, 0};
traced_object_insert(traced_objects, obj);
if (!traced_object_table_get(traced_objects, object))
{
GCTracedPointer obj = {object, 0, 0, 0};
traced_object_insert(traced_objects, obj);
// Make sure that this object is not in the thread's list as well.
unescaped_object_remove(unescaped, object);
}
}
else
{
unescaped_object_insert(unescaped, object);
if (!unescaped_object_table_get(unescaped, object))
{
unescaped_object_insert(unescaped, object);
}
}
}
pthread_rwlock_unlock(&traced_objects_lock);
@ -630,11 +658,19 @@ void GCAddObjectsForTracing(GCThread *thr)
// locking once.
id objc_assign_strongCast(id obj, id *ptr)
{
BOOL objIsDynamic = GCObjectIsDynamic(obj);
// This object is definitely stored somewhere, so mark it as visited
// for now.
if (obj)
if (objIsDynamic && obj)
{
GCSetFlag(obj, GCFlagVisited);
// Tracing semantics do not apply to objects with CF semantics, so skip the
// next bits if the CF flag is set.
if (obj && !GCTestFlag(obj, GCFlagCFObject))
{
// Don't free this just after scanning the stack.
GCSetFlag(obj, GCFlagEscaped);
}
}
pthread_rwlock_wrlock(&traced_objects_lock);
GCTracedPointer *old = traced_object_table_get(traced_objects, *ptr);
@ -652,18 +688,15 @@ id objc_assign_strongCast(id obj, id *ptr)
GCClearFlag(*ptr, GCFlagVisited);
}
}
GCTracedPointer *new = traced_object_table_get(traced_objects, obj);
if (new)
if (objIsDynamic && obj)
{
new->heapAddress = ptr;
GCTracedPointer *new = traced_object_table_get(traced_objects, obj);
if (new)
{
new->heapAddress = ptr;
}
}
pthread_rwlock_unlock(&traced_objects_lock);
// Tracing semantics do not apply to objects with CF semantics, so skip the
// next bits if the CF flag is set.
if (obj && !GCTestFlag(obj, GCFlagCFObject))
{
// Don't free this just after scanning the stack.
GCSetFlag(obj, GCFlagEscaped);
}
*ptr = obj;
return obj;
}

View File

@ -53,7 +53,7 @@
# define MAP_UNLOCK()
#else
# define MAP_LOCK() (LOCK(&table->lock))
# define MAP_UNLOCK() (ULOCK(&table->lock))
# define MAP_UNLOCK() (UNLOCK(&table->lock))
#endif
#ifndef MAP_TABLE_VALUE_TYPE
# define MAP_TABLE_VALUE_TYPE void*
@ -272,6 +272,7 @@ static int PREFIX(_insert)(PREFIX(_table) *table,
PREFIX(_table_cell) cell = PREFIX(_table_lookup)(table, hash);
if (MAP_TABLE_VALUE_NULL(cell->value))
{
cell->secondMaps = 0;
cell->value = value;
table->table_used++;
return 1;
@ -320,7 +321,6 @@ static int PREFIX(_insert)(PREFIX(_table) *table,
return 0;
}
__attribute__((unused))
static void *PREFIX(_table_get_cell)(PREFIX(_table) *table, void *key)
{
uint32_t hash = MAP_TABLE_HASH_KEY(key);
@ -354,17 +354,44 @@ static void *PREFIX(_table_get_cell)(PREFIX(_table) *table, void *key)
return NULL;
}
__attribute__((unused))
static void PREFIX(_table_move_second)(PREFIX(_table) *table,
PREFIX(_table_cell) emptyCell)
{
uint32_t jump = emptyCell->secondMaps;
// Look at each offset defined by the jump table to find the displaced location.
int hop = __builtin_ffs(jump);
PREFIX(_table_cell) hopCell =
PREFIX(_table_lookup)(table, MAP_TABLE_HASH_VALUE(emptyCell->value) + hop);
emptyCell->value = hopCell->value;
emptyCell->secondMaps &= ~(1 << (hop-1));
if (0 == hopCell->secondMaps)
{
hopCell->value = MAP_TABLE_VALUE_PLACEHOLDER;
}
else
{
PREFIX(_table_move_second)(table, hopCell);
}
}
__attribute__((unused))
static void PREFIX(_remove)(PREFIX(_table) *table, void *key)
{
uint32_t hash = MAP_TABLE_HASH_KEY((void*)key);
MAP_LOCK();
PREFIX(_table_cell) cell = PREFIX(_table_get_cell)(table, key);
if (NULL == cell) { return; }
cell->value = MAP_TABLE_VALUE_PLACEHOLDER;
// If the cell contains a value, set it to the placeholder and shuffle up
// everything
PREFIX(_table_move_gap)(table, hash + 32, hash, cell);
if (0 == cell->secondMaps)
{
cell->value = MAP_TABLE_VALUE_PLACEHOLDER;
}
else
{
PREFIX(_table_move_second)(table, cell);
}
table->table_used--;
MAP_UNLOCK();
}
__attribute__((unused))
@ -456,6 +483,26 @@ PREFIX(_next)(PREFIX(_table) *table,
return MAP_TABLE_VALUE_PLACEHOLDER;
#endif
}
/**
* Returns the current value for an enumerator. This is used when you remove
* objects during enumeration. It may cause others to be shuffled up the
* table.
*/
__attribute__((unused))
#ifdef MAP_TABLE_ACCESS_BY_REFERENCE
static MAP_TABLE_VALUE_TYPE*
#else
static MAP_TABLE_VALUE_TYPE
#endif
PREFIX(_current)(PREFIX(_table) *table,
struct PREFIX(_table_enumerator) **state)
{
#ifdef MAP_TABLE_ACCESS_BY_REFERENCE
return &(*state)->table->table[(*state)->index].value;
#else
return (*state)->table->table[(*state)->index].value;
#endif
}
#undef TABLE_SIZE
#undef REALLY_PREFIX_SUFFIX