Hi Theo,
Theo Buehler wrote on Sat, Jul 30, 2016 at 09:33:01PM +0200:
> ++#ifdef HAVE_EXPLICIT_BZERO
> ++#define CLEAR_PASSWORD(p, len) explicit_bzero((p), (len))
> ++#else
> ++#define CLEAR_PASSWORD(p, len) memset((p), 0, (len))
> ++#endif
Not to be a prick or to start a bikeshed - i'm not opposed to your
patch. Just for consideration, more for encouraging best practices
in the general case than in this specific example.
I think we should encourage upstream projects to handle portability
in the following way:
1. Fully commit to using good interfaces in the main code,
no matter whether they are (already) standardized.
NEVER use wrappers around standard interfaces, and AVOID
wrappers even around good non-standard interfaces.
2. Instead, provide replacement implementations in compat_*.c
files at the edge of the upstream codebase, if possible
containing just a copy of the best available implementation,
if impossible containing an implementation with reduced
functionality, in extreme cases where even that is
impossible, like for pledge(2), a no-op stub.
Hence, why not:
[...]
> +@@ -172,10 +178,12 @@ readpw(Display *dpy, const char *pws)
> + break;
> + case XK_Escape:
> + len = 0;
> ++ explicit_bzero(passwd, sizeof(passwd));
> + break;
and so on? Encourage upstream to take that as-is.
Proposing HAVE_EXPLICIT_BZERO to upstream is very good. Do that!
Then encourage upstream to:
1. Include a file compat_explicit_bzero.c into their tarball.
Ideally, talk to some of our C wizards (like tedu@) how
a portable implementation should look like. I suspect
that src/lib/libc/string/explicit_bzero.c will not be
very portable and maybe not even work outside libc.
2. Include a configuration test checking at least whether the
operating system provides a function explicit_bzero(3) with the
correct prototype in the correct header, ideally also whether
it actually zeros the memory in some situation where some
compilers would optimize a memset(3) away.
3. If step 2 succeeds, let whatever configuration system upstream
uses set HAVE_EXPLICIT_BZERO; otherwise, let it clear
HAVE_EXPLICIT_BZERO, thus choosing which implementation
to use. Look at
http://mdocml.bsd.lv/cgi-bin/cvsweb/?cvsroot=mdocml
For many examples of test-*.c files and compat_*.c
implementations, though admittely not including
explicit_bzero(3).
If upstream does not want advice 1, tell them to use this:
> ++#ifndef HAVE_EXPLICIT_BZERO
> ++#define explicit_bzero(p, len) memset((p), 0, (len))
> ++#endif
Yours,
Ingo
