s4-messaging: Use generate_random() to get a unique ID for messaging clients

[simo]

On Wed, 2012-04-25 at 22:51 +1000, Andrew Bartlett wrote: 
> On Wed, 2012-04-25 at 08:04 -0400, simo wrote:
> > On Wed, 2012-04-25 at 11:44 +0200, Andrew Bartlett wrote:
> > > commit b8055132b1c62dd19981fea2822ab9e1829a8ded
> > > Author: Andrew Bartlett <abartlet at samba.org>
> > > Date:   Wed Apr 25 17:53:18 2012 +1000
> > > 
> > >     s4-messaging: Use generate_random() to get a unique ID for
> > > messaging clients
> > >     
> > >     The call to random() resulted in duplicate values for s3fs
> > > configurations
> > >     which, due to the forked child, all started with the same random
> > > seed.
> > >     
> > >     A future improvement would be to move to a proven unique value.
> > >     
> > >     Andrew Bartlett
> > >     
> > >     Autobuild-User: Andrew Bartlett <abartlet at samba.org>
> > >     Autobuild-Date: Wed Apr 25 11:43:40 CEST 2012 on sn-devel-104
> > 
> > 
> > Andrew are you sure you need to use /dev/urandom here ? It doesn't look
> > like you need absolutelu unpredictable numbers here, just non-colliding
> > numbers.
> > You changed the code to draw from urandom, and if it is used often it
> > mean it will suck a lot of entropy out of the system, causing any
> > application that need to use /dev/random to halt.
> > Wouldn't it have been simpler to just run srand(time(NULL)*pid) to get a
> > new seed for the process ?
> 
> Simo,
> 
> You are correct to think carefully about our over-use of random entropy.
> In this case, it isn't used often (typically once per child process in
> s3fs configurations only), and we have logic in generate_random_buffer()
> to attempt to reduce the load on /dev/urandom. We set up our own PRNG,
> where a process has read more than 40 bytes, which I added with
> 6554433fc227baab93398576db703c91db1541f2 back in 2007.

This doesn't really help at process startup, as we reset the global
variable after fork, so for the first 40 bytes at least we do
use /dev/urandom, that's why I had a concern.

> I also worked on a solution using IDR trees to allocate a server_id
> number (along side the pid, for the auth_samba4 use case):
> https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/s3-task-id-allocator however I haven't yet made it work.  If I can make it work, I would prefer to use an allocator, for more predictable behaviour under test.  
> 
> Another idea I had was to use the combination of pid and memory address
> of the event context or messaging context, but currently 'task' is just
> 32 bits.
> 
> Metze has also suggested some improvements on which fields to randomize
> in https://bugzilla.samba.org/show_bug.cgi?id=8872#c4 

Ok, seem like iot is staying on the radar, that's good enough.

> As to if we should use random() and srand():  I would rather not be
> reseeding the srand() as it may be deliberately set by smbtorture
> callers wishing to create reproducible runs, and the messaging client is
> used by smbtorture. 

True, although libraries could change it, so depending on nothing ever
calling srand() in smbtorture seem a bit fragile.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>