Re: [PATCH RFC] explicit_bzero, again

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 16-08-2015 13:43, Zack Weinberg wrote:
> On 08/09/2015 07:29 PM, Zack Weinberg wrote:
>> I'd like to resume the discussion of adding explicit_bzero or similar
>> to glibc.  This was originally proposed by Nick Mathewson last
>> December (https://sourceware.org/ml/libc-alpha/2014-12/msg00506.html)
>> and I have prepared an updated patchset, which is attached to this
>> message.  One patch adds the function, and the second adds several
>> intra-libc uses; I suspect I haven't found all of the places where
>> it should be used.
> 
> This updated patchset corrects a number of minor errors (thanks,
> Joseph); adds tests for the fortification of explicit_bzero; and revises
> the new manual section a little.
> 
> zw
> 

As Joseph I also support the principle of adding this new API.  Regarding the
patch some comments below.  It would be better if you send it as inline instead
of attachment, and send on message for each patch.

> +#ifdef __USE_MISC
> +# if __GNUC_PREREQ (4, 0) && !defined __clang__ && !defined __cplusplus
> +__STRING_INLINE void
> +__explicit_bzero_constn (void *__s, size_t __n)
> +{
> +  typedef struct {char __x[__n];} __memblk;
> +  memset (__s, 0, __n);
> +  __asm__ ("" : : "m" (*(__memblk __attribute__ ((__may_alias__)) *)__s));
> +}
> +
> +#  define explicit_bzero(s, n)                          \
> +  (__extension__ (__builtin_constant_p (n) && (n) > 0   \
> +                  ? __explicit_bzero_constn (s, n)      \
> +                  : explicit_bzero (s, n)))
> +# endif

I noted that Linux kernel uses the following definition:

#define barrier_data(ptr) __asm__ __volatile__("": :"r"(ptr) :"memory")

With the justification that:

 * This version is i.e. to prevent dead stores elimination on @ptr
 * where gcc and llvm may behave differently when otherwise using
 * normal barrier(): while gcc behavior gets along with a normal
 * barrier(), llvm needs an explicit input variable to be assumed
 * clobbered.

Would be better to use kernel definition and drop the !clang instead?
Same applies for explicit_bzero at 'string/bits/string3.h'.

> +extern void __explicit_bzero_chk (void *__dest, size_t __len, size_t __dstlen);
> +extern void __REDIRECT_NTH (__explicit_bzero_alias,
> +                            (void *__dest, size_t __len),
> +                            explicit_bzero);
> +
> +__fortify_function void
> +__NTH (explicit_bzero (void *__dest, size_t __len))
> +{
> +# if __GNUC_PREREQ (4, 0) && !defined __clang__ && !defined __cplusplus
> +  if (__builtin_constant_p (__len) && __len > 0)
> +    {
> +      (void) __builtin___memset_chk (__dest, '\0', __len, __bos0 (__dest));
> +      typedef struct { char __x[__len]; } __memblk;
> +      __asm__ ("" :: "m" (*(__memblk __attribute__ ((__may_alias__)) *)__dest));
> +    }
> +  else
> +# endif
> +  if (__bos0 (__dest) != (size_t) -1
> +      && (!__builtin_constant_p (__len) || __len > __bos0 (__dest)))
> +    __explicit_bzero_chk (__dest, __len, __bos0 (__dest));
> +  else
> +    __explicit_bzero_alias (__dest, __len);
> +}

Do we want to 'chk' version being use even for non-check? Also I see that
OpenBSD added a patch to try disable this option for LTO [1].  Do you think
could it be worth to add as well in your version?

[1] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/string/explicit_bzero.c?rev=1.3&content-type=text/x-cvsweb-markup