[Openal-devel] Something dangerous-looking in linux impl.

[Adam D. Moss]

Code below excerpted from al_buffer.c.

This looks like it might be potentially hazardous in a gnarly way.
Watch the use of buf->queue_list.items: in the first instance
it's being used as the 'next free position' cursor, but in
the second instance it's being used like a refcount.
_alBufferRemoveQueueRef() is written as though the sid to be
removed could occur anywhere in the queue_list, BUT
_alBufferAddQueueRef() will then possibly overwrite a
still-valid list entry if that previously removed entry
wasn't the very last sid in the list (the free-position cursor
has been moved one slot down from the end of the list, but
that doesn't necessarily correspond to the newly-vacated
slot).

As a second observation, since the realloc'd list in
_alBufferAddQueueRef() grows geometrically it will, after the
initial grow, be left with heap-noise in its final slots.  This
might potentially allow false-positive matches in
_alBufferRemoveQueueRef() and over-decrement the refcount.

I think that this can all only be of relevance when a buffer is
queued multiple times and/or on multiple sources.  (n.b. so is
unlikely to be related to my own problem.)  An identical set
of gotchas applies to _alBuffer{Add,Remove}CurrentRef() also.


static void _alBufferAddQueueRef( AL_buffer *buf, ALuint sid ) {
	ALvoid *temp;
	ALuint newsize = buf->queue_list.size;

	if(buf->queue_list.size <= buf->queue_list.items) {
		/* resize */
		newsize *= 2;
		newsize += 1;
		temp = realloc(buf->queue_list.sids, newsize * sizeof *buf->queue_list.sids);
		if(temp == NULL) {
			/* well la-di-da */
			return;
		}

		buf->queue_list.sids = temp;
		buf->queue_list.size = newsize;
	}

	buf->queue_list.sids[buf->queue_list.items++] = sid;

	return;
}

[....]

/*
  * _alBufferRemoveQueueRef( AL_buffer *buf, ALuint sid )
  *
  * removes the first queue reference from the AL_buffer *buf that refers to
  * sid.
  *
  * assumes locked context, buffer
  */
static void _alBufferRemoveQueueRef( AL_buffer *buf, ALuint sid ) {
	ALuint i;

	for(i = 0; i < buf->queue_list.size; i++) {
		if(buf->queue_list.sids[i] == sid) {
			buf->queue_list.sids[i] = 0;
			buf->queue_list.items--;

			return;
		}
	}

	return;
}


-- 
Adam D. Moss   . ,,^^   adam at gimp.org   http://www.foxbox.org/   co:3
"At this point the rocket becomes engorged with astronauts."