[Asterisk-Dev] Race issue in channel.c involving uniqueint on Asterisk 1.2.1

[Luigi Rizzo]

On Fri, Dec 30, 2005 at 12:21:31PM +0100, Werner Johansson wrote:
> Hello everyone,
> 
> We have faced a problem with non-unique UniqueID:s attached to channels when 
> originating lots of calls at once in async mode using the manager interface. 
> This creates problems for us trying to map channel events like link and call 
> progress to a specific ActionID being supplied to the Originate action. 
> Tracking down the code that calculates the uniqueid (in channel.c) and 
> looking at the code generated by the compiler it is obvious that this piece 
> of code isn't thread safe (non-atomic increment of the call id). If this 

in freebsd, and i believe other systems as well, there is a set of
functions that do atomic manipulation of variables e.g.
(from i386/include/atomic.h):
/*
 * Various simple arithmetic on memory which is atomic in the presence
 * of interrupts and multiple processors.
 *
 * atomic_set_char(P, V)        (*(u_char*)(P) |= (V))
 * atomic_clear_char(P, V)      (*(u_char*)(P) &= ~(V))
 * atomic_add_char(P, V)        (*(u_char*)(P) += (V))
 * atomic_subtract_char(P, V)   (*(u_char*)(P) -= (V))
 *
 * atomic_set_short(P, V)       (*(u_short*)(P) |= (V))
 * atomic_clear_short(P, V)     (*(u_short*)(P) &= ~(V))
 * atomic_add_short(P, V)       (*(u_short*)(P) += (V))
 * atomic_subtract_short(P, V)  (*(u_short*)(P) -= (V))
 *
 * atomic_set_int(P, V)         (*(u_int*)(P) |= (V))
 * atomic_clear_int(P, V)       (*(u_int*)(P) &= ~(V))
 * atomic_add_int(P, V)         (*(u_int*)(P) += (V))
 * atomic_subtract_int(P, V)    (*(u_int*)(P) -= (V))
 * atomic_readandclear_int(P)   (return  *(u_int*)P; *(u_int*)P = 0;)
 *
 * atomic_set_long(P, V)        (*(u_long*)(P) |= (V))
 * atomic_clear_long(P, V)      (*(u_long*)(P) &= ~(V))
 * atomic_add_long(P, V)        (*(u_long*)(P) += (V))
 * atomic_subtract_long(P, V)   (*(u_long*)(P) -= (V))
 * atomic_readandclear_long(P)  (return  *(u_long*)P; *(u_long*)P = 0;)
 */

which are then translated in an architecture-dependent way to the
proper code. FreeBSD has code for a lot of architectures
(this is an architecture issue, not an OS issue).

While a sequence

	lock(&foo_lock);
	x = foo++;
	unlock(&foo_lock);

does the job in the general case, it is really a very expensive
way to solve the problem.

I think the proper course of action is to wrap the atomic ops
into macros, and then let the common header implement the
locking in the proper way, with fallback to the above
sequence only for unknown architectures.

FreeBSD (and i suppose linux as well) has example code
for i386 and others in the machine/atomic.[ch] files

	cheers
	luigi

> happens before the time variable changes there will be more than one channel 
> with the same ID, which obviously is bad. If the UniqueID is never used this 
> probably doesn't affect operation, but for us that is the only way to see 
> which channel belongs to which call, as the ActionID isn't passed down to 
> the actual channel structure (i.e we only get ActionID on OriginateSuccess, 
> but never on Newstate for instance).
> 
> This has been spotted in Asterisk 1.2.1, but no changes to this code can be 
> seen in the newer revisions on channel.c in the trunk or on the 1.2 branch. 
> We have developed a quick fix which seems to work, but there might be some 
> other way to implement this. Diff included below, comments appreciated!
> 
> Should this be reported as a bug or has it already been seen elsewhere? I 
> haven't seen uniqueint being mentioned as a source of problems before.
> 
> Best Regards,
> Werner Johansson
> 
> 
> 
> *** channel.c   2005-12-02 00:34:58.000000000 +0100
> --- ../../ast1.2_rh/asterisk-1.2.1/channel.c    2005-12-29 
> 22:48:24.717558008 +0100
> ***************
> *** 97,100 ****
> --- 97,108 ----
>   static int uniqueint = 0;
> 
> + /* Rådhuset edit starts - CJS 051229 21:45 */
> +
> + AST_MUTEX_DEFINE_STATIC(ast_unique_increase_lock);
> +
> + /* Rådhuset edit ends - CJS 051229 21:45 */
> +
> +
> +
>   unsigned long global_fin = 0, global_fout = 0;
> 
> ***************
> *** 584,588 ****
>         tmp->fin = global_fin;
>         tmp->fout = global_fout;
> !       snprintf(tmp->uniqueid, sizeof(tmp->uniqueid), "%li.%d", (long) 
> time(NULL), uniqueint++);
>         headp = &tmp->varshead;
>         ast_mutex_init(&tmp->lock);
> --- 592,606 ----
>         tmp->fin = global_fin;
>         tmp->fout = global_fout;
> !
> !       /* Rådhuset edit starts - CJS 051229 21:45 */
> !
> !       ast_mutex_lock(&ast_unique_increase_lock);
> !       uniqueint++;
> !       snprintf(tmp->uniqueid, sizeof(tmp->uniqueid), "%li.%d", (long) 
> time(NULL),uniqueint);
> !       ast_mutex_unlock(&ast_unique_increase_lock);
> !
> !       /* Rådhuset edit ends - CJS 051229 21:45 */
> !
> !
>         headp = &tmp->varshead;
>         ast_mutex_init(&tmp->lock);
> 
> _______________________________________________
> --Bandwidth and Colocation provided by Easynews.com --
> 
> Asterisk-Dev mailing list
> To UNSUBSCRIBE or update options visit:
>    http://lists.digium.com/mailman/listinfo/asterisk-dev