Re: [PATCH 5/5] Add arc4random() etc. from OpenBSD 5.8

[Sebastian Huber <sebastian dot huber at embedded-brains dot de>]

----- Am 18. Mrz 2016 um 18:01 schrieb Corinna Vinschen vinschen@redhat.com:

> On Mar 18 17:25, Corinna Vinschen wrote:
>> On Mar 18 15:48, Sebastian Huber wrote:
>> > On 18/03/16 14:50, Corinna Vinschen wrote:
>> > >On Mar 18 12:28, Corinna Vinschen wrote:
>> > >>>On Mar 18 11:49, Sebastian Huber wrote:
>> > >>>> >According to the OpenBSD man page, "A Replacement Call for Random".  It
>> > >>>> >offers high quality random numbers derived from input data obtained by
>> > >>>> >the OpenBSD specific getentropy() system call which is declared in
>> > >>>> ><unistd.h> and must be implemented for each Newlib port externally.  The
>> > >>>> >arc4random() functions are used for example in LibreSSL and OpenSSH.
>> > >>>> >
>> > >>>> >Cygwin provides currently its own implementation of the arc4random
>> > >>>> >family.  Maybe it makes sense to use this getentropy() implementation:
>> > >>>> >
>> > >>>> >http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/crypto/getentropy_win.c?rev=1.4&content-type=text/x-cvsweb-markup
>> > >>>
>> > >>>No, that pulls in a dependency to another DLL which we're trying to avoid.
>> > >>>Using RtlGenRandom should work though.  I have to test this.
>> > >All patches applied.  I immediately implemented the changes required for
>> > >Cygwin.  In the first place that required to add two functions
>> > >arc4random_stir and arc4random_addrandom to maintain backward
>> > >compatibility since both OpenBSD functions were already exported by
>> > >Cygwin.
>> > 
>> > My aim with the "newlib/libc/sys/*/include/machine/_arc4random.h" file was
>> > to avoid #ifdef SYS_XYZ cascades in the generic files. It seems that for
>> > Cygwin there is no "newlib/libc/sys/cygwin" directory. Would it be possible
>> > to add this and place a
>> > "newlib/libc/sys/cygwin/include/machine/_arc4random.h" in it?
>> 
>> That might be possible, but the arc4random.h file does not even have
>> provisions for redefining _ARC4_LOCK/_ARC4_UNLOCK.  The file defines
>> them unconditionally.
> 
> Patch proposal:
> 
> commit dc12772137ad6fd65f8628e4f8b0625163e46e28
> Author: Corinna Vinschen <corinna@vinschen.de>
> Date:   Fri Mar 18 18:01:07 2016 +0100
> 
>    Allow machine-dependent arc4 locking
>    
>    newlib:
>    	* libc/stdlib/arc4random.h: Remove Cygwin-specific locking code.
>    	Conditionalize arc4 locking.  Check for _ARC4_LOCK_INIT being
>    	undefined to fall back to default implementation.
>    
>    cygwin:
>    	* include/machine/_arc4random.h: New file.
>    
>    Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
> 
> diff --git a/newlib/libc/stdlib/arc4random.h b/newlib/libc/stdlib/arc4random.h
> index 8bb72f4..54bcbe8 100644
> --- a/newlib/libc/stdlib/arc4random.h
> +++ b/newlib/libc/stdlib/arc4random.h
> @@ -37,30 +37,17 @@
> #include <sys/lock.h>
> #include <signal.h>
> 
> -__LOCK_INIT(static, _arc4random_mutex);
> +#ifndef _ARC4_LOCK_INIT
> 
> -#ifdef __CYGWIN__
> -
> -extern int __isthreaded;
> -
> -#define _ARC4_LOCK()				\
> -        do {					\
> -	  if (__isthreaded)			\
> -	    __lock_acquire (_arc4random_mutex);	\
> -        } while (0)
> -
> -#define _ARC4_UNLOCK()				\
> -        do {					\
> -	  if (__isthreaded)			\
> -	    __lock_release (_arc4random_mutex);	\
> -        } while (0)
> -#else
> +#define _ARC4_LOCK_INIT __LOCK_INIT(static, _arc4random_mutex);
> 
> #define _ARC4_LOCK() __lock_acquire(_arc4random_mutex)
> 
> #define _ARC4_UNLOCK() __lock_release(_arc4random_mutex)
> 
> -#endif
> +#endif /* _ARC4_LOCK_INIT */
> +
> +_ARC4_LOCK_INIT
> 
> #ifdef _ARC4RANDOM_DATA
> _ARC4RANDOM_DATA
> diff --git a/winsup/cygwin/include/machine/_arc4random.h
> b/winsup/cygwin/include/machine/_arc4random.h
> new file mode 100644
> index 0000000..3ce2458
> --- /dev/null
> +++ b/winsup/cygwin/include/machine/_arc4random.h
> @@ -0,0 +1,30 @@
> +/* machine/_arc4random.h
> +
> +   Copyright 2016 Red Hat, Inc.
> +
> +This file is part of Cygwin.
> +
> +This software is a copyrighted work licensed under the terms of the
> +Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
> +details. */
> +
> +#ifndef _MACHINE_ARC4RANDOM_H
> +#define _MACHINE_ARC4RANDOM_H
> +
> +extern int __isthreaded;
> +
> +#define _ARC4_LOCK_INIT	__LOCK_INIT(static, _arc4random_mutex);
> +
> +#define _ARC4_LOCK()				\
> +        do {					\
> +	  if (__isthreaded)			\
> +	    __lock_acquire (_arc4random_mutex);	\
> +        } while (0)
> +
> +#define _ARC4_UNLOCK()				\
> +        do {					\
> +	  if (__isthreaded)			\
> +	    __lock_release (_arc4random_mutex);	\
> +        } while (0)
> +
> +#endif /* _MACHINE_ARC4RANDOM_H */

Out of curiosity, why is this __isthreaded stuff not covered by <sys/lock.h> or only necessary here, e.g. in contrast to for example the atexit lock?