[asterisk-dev] [Code Review]: Add UUID support to Asterisk

Wed Nov 28 16:58:48 CST 2012

> On Nov. 28, 2012, 11:53 a.m., David Lee wrote:
> > /trunk/main/uuid.c, line 175
> > <https://reviewboard.asterisk.org/r/2217/diff/2/?file=32250#file32250line175>
> >
> >     It would be useful to test for the existing of /dev/urandom and log an error if it doesn't exist.
> >     
> >     /dev/random is a blocking entropy source, and could dramatically affect performance if that's the only generator available.
> >     
> >     If neither are available, libuuid uses rand(), which isn't thread safe.
> 
> Mark Michelson wrote:
>     This comment got me looking into libuuid a bit more.
>     
>     If libuuid uses /dev/random/, it actually sets the reads to be nonblocking. If it fails to read data 16 times in a row, then it will fall back to simply using rand() for the current UUID instead. So performance is not degraded as much as it might be if it blocked while trying to read from /dev/random/
>     
>     I took a look at the libuuid source a bit further, and even when getting data from /dev/random/ or /dev/urandom/, it actually uses the rand() (and possibly some use of jrand48()) function to do some adjustment on the returned bytes. If rand() is not thread-safe, then it would appear that *all* calls to uuid_generate_random() are not thread-safe due to their unprotected calls to rand().
>     It appears that we may need to surround all calls to uuid_generate_random() with a mutex. This may hurt performance some, but given all the locking Asterisk already is doing, I don't think this will be a bottleneck. What do you think?
> 
> David Lee wrote:
>     In practice, if we're using /dev/urandom, this isn't likely to be a problem. libuuid is essentially generating a high quality, 128-bit random number. It then proceeds to XOR that number with a low quality random number generated by rand(). Since XOR preserves entropy, you still have a high quality random number.
>     
>     This assumes that the thread-unsafe misbehaviors are minor (rand() isn't as random as it should be) and not major (rand() sends a message to SkyNet to start the revolution, posts your bank account information to Twitter, and then formats your hard drive, all the while displaying a laughing, highly pixelated Jolly Roger on the screen).
>     
>     I dislike the idea of this becoming a contention point in the system, since it will likely be used by several different components. Although, the lock would help in the off chance libuuid falls back to using rand(). Maybe only lock if /dev/urandom is unavailable. Or maybe that's an optimization we can do if we decide that contention on generating UUID's is causing us trouble.
>     
>     Sorry, I have no answers for you. Just vague ramblings.

The manpage for rand says this:

"The function rand() is not reentrant or thread-safe, since it uses hidden state that is modified on each call. This might just be the seed value to be used by the next call, or it might be something more elaborate. In order to get reproducible behavior in a threaded application, this state must be made explicit; this can be done using the reentrant function rand_r()."

This is a touch vague, but the only thing it mentions as far as the consequences of its non-thread-safety is that you cannot reliably reproduce results in multi-threaded applications unless the reentrant version is used. It doesn't mention anything about potential undefined behavior, so I have confidence the Skynet scenario you mentioned is unlikely.

Interestingly, have a look at Asterisk's ast_random() function in main/utils.c. It uses /dev/urandom/ if possible, but otherwise it will call random(). If on Linux, this is not protected with a lock, but on non-Linux systems, there is a lock around the random() call. What does this tell me? Not much, really, but still it's interesting.

- Mark

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2217/#review7461
-----------------------------------------------------------

On Nov. 28, 2012, 10:16 a.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2217/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2012, 10:16 a.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and David Lee.
> 
> 
> Summary
> -------
> 
> This is the first of many parts of the overall goal of improving Asterisk APIs. This adds a UUID API to Asterisk, eventually to be used as a method of uniquely identifying channels and potentially other objects.
> 
> The implementation here uses libuuid, which is packaged by all major Linux distributions. The API is basically a wrapper around libuuid's API, but it hides all implementation details in case we want to switch out libraries at some point.
> 
> Licensing for libuuid is a bit confusing. On one hand, this online man page (http://linux.die.net/man/3/libuuid) claims that the library is distributed under the LGPL version 2. However, when I downloaded the source for the library on my machine, the code had the 3-clause BSD license printed in the source. In either case, I believe use of external library is license-compatible with Asterisk.
> 
> Regarding specific implementation decisions, I think comments in the code should explain why I chose to do things certain ways. Please let me know if anything should be done differently or if you have ideas for tests beyond what I have already included.
> 
> I have no idea what is up with the changes listed in autoconfig.h.in. Those changes showed up when I ran the bootstrap.sh script after changing configure.ac. 
> 
> 
> This addresses bug ASTERISK-20725.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20725
> 
> 
> Diffs
> -----
> 
>   /trunk/configure UNKNOWN 
>   /trunk/configure.ac 376724 
>   /trunk/include/asterisk/autoconfig.h.in 376724 
>   /trunk/include/asterisk/uuid.h PRE-CREATION 
>   /trunk/main/Makefile 376724 
>   /trunk/main/asterisk.c 376724 
>   /trunk/main/uuid.c PRE-CREATION 
>   /trunk/tests/test_uuid.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2217/diff
> 
> 
> Testing
> -------
> 
> I have included a set of unit tests that exercise all the APIs. They pass when run locally.
> 
> 
> Thanks,
> 
> Mark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20121128/7ea8d755/attachment-0001.htm>