[asterisk-dev] Locking in astmm.c

[Matt Roth]

List members,

While converting trunk to use Russell's *alloc wrappers, I came across 
some tricky code in astmm.c.  Before I make any changes, I would 
appreciate your input.

In __ast_alloc_region, some memory is alloc'd, a lock is acquired, then 
the allocation is tested.  Here is the body of the function:

======================================================================

static inline void *__ast_alloc_region(size_t size, int which, const 
char *file, int lineno, const char *func)
{
    struct ast_region *reg;
    void *ptr = NULL;
    unsigned int *fence;
    int hash;
    reg = malloc(size + sizeof(struct ast_region) + sizeof(unsigned int));
    ast_mutex_lock(&reglock);
    if (reg) {
        ast_copy_string(reg->file, file, sizeof(reg->file));
        reg->file[sizeof(reg->file) - 1] = '\0';
        ast_copy_string(reg->func, func, sizeof(reg->func));
        reg->func[sizeof(reg->func) - 1] = '\0';
        reg->lineno = lineno;
        reg->len = size;
        reg->which = which;
        ptr = reg->data;
        hash = HASH(ptr);
        reg->next = regions[hash];
        regions[hash] = reg;
        reg->fence = FENCE_MAGIC;
        fence = (ptr + reg->len);
        *fence = FENCE_MAGIC;
    }
    ast_mutex_unlock(&reglock);
    if (!reg) {
        fprintf(stderr, "Out of memory :(\n");
        if (mmlog) {
            fprintf(mmlog, "%ld - Out of memory\n", time(NULL));
            fflush(mmlog);
        }
    }
    return ptr;
}

======================================================================

I believe it is safe to rewrite the function as follows.  Note that the 
only differences are:

1) The memory is now alloc'd, the allocation is tested, and the LOCK IS 
ACQUIRED LAST.
2) The lock is not acquired and unlocked if the allocation fails.

======================================================================

static inline void *__ast_alloc_region(size_t size, const enum func_type 
which, const char *file, int lineno, const char *func)
{
    struct ast_region *reg;
    void *ptr;
    unsigned int *fence;
    int hash;

    if (!(reg = ast_malloc(size + sizeof(*reg) + sizeof(*fence)))) {
        fprintf(stderr, "Out of memory :(\n");
        if (mmlog) {
            fprintf(mmlog, "%ld - Out of memory\n", time(NULL));
            fflush(mmlog);
        }        
        return NULL;
    }
    
    ast_mutex_lock(&reglock);
    ast_copy_string(reg->file, file, sizeof(reg->file));
    reg->file[sizeof(reg->file) - 1] = '\0';
    ast_copy_string(reg->func, func, sizeof(reg->func));
    reg->func[sizeof(reg->func) - 1] = '\0';
    reg->lineno = lineno;
    reg->len = size;
    reg->which = which;
    ptr = reg->data;
    hash = HASH(ptr);
    reg->next = regions[hash];
    regions[hash] = reg;
    reg->fence = FENCE_MAGIC;
    fence = (ptr + reg->len);
    *fence = FENCE_MAGIC;
    ast_mutex_unlock(&reglock);
    
    return ptr;
}

======================================================================

I think the lock protects the regions array, which is a shared hash 
table of linked lists of ast_regions.  My concern is that the original 
code appears to be specifically written to acquire the lock prior to 
testing the allocation.  That seems odd, because I don't believe the 
pointer that is returned by the allocation can be modified by other 
threads, but sometimes Asterisk works in mysterious ways.  : )

Does my change look safe, or am I introducing a race condition?

Thank you,

Matthew Roth
InterMedia Marketing Solutions
Software Engineer and Systems Developer