[asterisk-dev] pthread_getspecific() in app_voicemail

[Tilghman Lesher]

On Wednesday 15 April 2009 16:52:17 Mark Sirota wrote:
> A recent version introduced new code to app_voicemail, using
> pthread_setspecific() and pthread_getspecific() to handle vm_state in some
> circumstances.
>
> This code has introduced a bug -- sometimes pthread_getspecific() is called
> before pthread_setspecific().  It has been reported here in more detail:
>
> <http://bugs.digium.com/view.php?id=14889>
>
>
> I have a simple patch, involving setting a flag when calling
> pthread_setspecific() and checking it before calling pthread_getspecific().
> However, I have no idea if this is the best approach, because I don't know
> why the pthread calls were introduced.
>
> I can't find a report in the issue tracker related to this.  Can anyone
> tell me what problem the pthread code solves?

The problem is that the IMAP code does some extremely unsafe things.  First,
it was storing the interactive sessions in a global list that was accessible
to noninteractive sessions.  This introduces a race condition whereby a
noninteractive thread can access stack space after the original routine has
exited and another routine may be accessing the same stack space.

Secondarily, if two users accessed the same mailbox at the same time (due to
the sharing of structures in a global list), then similarly a race condition
resulted that could crash Asterisk, for much the same reason.

I don't see how calling pthread_getspecific() before pthread_setspecific() is
called could be a problem.  According to the POSIX specification (and the
Linux manpage), pthread_getspecific() should return NULL if
pthread_setspecific() is not called first.  Additionally, the thread which
calls pthread_setspecific() to set the value to the location in the stack,
calls it again at the end of the routine, to set it back to NULL.  So there
should be no situation that a thread is accessing that vm_state structure
that was stored on the thread stack.

Additionally note that if pthread_getspecific() returns NULL, then a structure
is allocated from the heap, which is stored in the global list of vm_state
structures.  There's actually still an additional problem, in that the stored
mailstream structures are shared haphazardly, so cleaning them up is near
impossible.  I understand that it was done originally for speed, but the
result is structures that can never be deallocated (and a TON of them, too)
without introducing instability.

-- 
Tilghman