Adopt a new universal implementation of smemclr().

This new implementation uses the same optimisation-barrier technique
that I used in various places in testsc: have a no-op function, and a
volatile function pointer pointing at it, and then call through the
function pointer, so that nothing actually happens (apart from the
physical call and return) but the compiler has to assume that
_anything_ might have happened.

Doing this just after a memset enforces that the compiler can't have
thrown away the memset, because the called function might (for
example) check that all the memory really is zero and abort if not.

I've been turning this over in my mind ever since coming up with the
technique for testsc. I think it's far more robust than the previous
smemclr technique: so much so that I'm switching to using it
_everywhere_, and no longer using platform alternatives like Windows's
SecureZeroMemory().

CMakeLists.txt

@@ -40,6 +40,7 @@ add_library(utils STATIC
   utils/seat_connection_fatal.c
   utils/sessprep.c
   utils/sk_free_peer_info.c
+  utils/smemclr.c
   utils/smemeq.c
   utils/ssh2_pick_fingerprint.c
   utils/sshutils.c

cmake/platforms/windows.cmake

@@ -36,9 +36,6 @@ define_negation(NO_MULTIMON HAVE_MULTIMON_H)
 check_include_files("windows.h;htmlhelp.h" HAVE_HTMLHELP_H)
 define_negation(NO_HTMLHELP HAVE_HTMLHELP_H)
 
-check_symbol_exists(SecureZeroMemory "windows.h" HAVE_SECUREZEROMEMORY)
-define_negation(NO_SECUREZEROMEMORY HAVE_SECUREZEROMEMORY)
-
 check_symbol_exists(strtoumax "inttypes.h" HAVE_STRTOUMAX)
 check_symbol_exists(AddDllDirectory "windows.h" HAVE_ADDDLLDIRECTORY)
 check_symbol_exists(SetDefaultDllDirectories "windows.h"

defs.h

@@ -201,32 +201,4 @@ typedef struct PacketProtocolLayer PacketProtocolLayer;
 #define NORETURN
 #endif
 
-/* ----------------------------------------------------------------------
- * Platform-specific definitions.
- *
- * Most of these live in the per-platform header files, of which
- * puttyps.h selects the appropriate one. But some of the sources
- * (particularly standalone test applications) would prefer not to
- * have to include a per-platform header at all, because that makes it
- * more portable to platforms not supported by the code base as a
- * whole (for example, compiling purely computational parts of the
- * code for specialist platforms for test and analysis purposes). So
- * any definition that has to affect even _those_ modules will have to
- * go here, with the key constraint being that this code has to come
- * to _some_ decision even if the compilation platform is not a
- * recognised one at all.
- */
-
-/* Purely computational code uses smemclr(), so we have to make the
- * decision here about whether that's provided by utils.c or by a
- * platform implementation. We define PLATFORM_HAS_SMEMCLR to suppress
- * utils.c's definition. */
-#ifdef _WINDOWS
-/* Windows provides the API function 'SecureZeroMemory', which we use
- * unless the user has told us not to by defining NO_SECUREZEROMEMORY. */
-#ifndef NO_SECUREZEROMEMORY
-#define PLATFORM_HAS_SMEMCLR
-#endif
-#endif
-
 #endif /* PUTTY_DEFS_H */

unix/CMakeLists.txt

@@ -18,7 +18,6 @@ add_platform_sources_to_library(utils
   utils/pollwrap.c
   utils/signal.c
   utils/x11_ignore_error.c
-  ../utils/smemclr.c
   # Compiled icon pixmap files
   xpmpucfg.c xpmputty.c xpmptcfg.c xpmpterm.c
   # We want the ISO C implementation of ltime(), because we don't have

utils/buildinfo.c

@@ -130,9 +130,6 @@ char *buildinfo(const char *newline)
 #if defined _WINDOWS && defined MINEFIELD
     strbuf_catf(buf, "%sBuild option: MINEFIELD", newline);
 #endif
-#ifdef NO_SECUREZEROMEMORY
-    strbuf_catf(buf, "%sBuild option: NO_SECUREZEROMEMORY", newline);
-#endif
 #ifdef NO_IPV6
     strbuf_catf(buf, "%sBuild option: NO_IPV6", newline);
 #endif

utils/smemclr.c

@@ -6,18 +6,36 @@
  * won't optimise away memsets on variables that are about to be freed
  * or go out of scope. See
  * https://buildsecurityin.us-cert.gov/bsi-rules/home/g1/771-BSI.html
- *
- * Some platforms (e.g. Windows) may provide their own version of this
- * function.
  */
 
 #include "defs.h"
 #include "misc.h"
 
+/*
+ * Trivial function that is given a pointer to some memory and ignores
+ * it.
+ */
+static void no_op(void *ptr, size_t size) {}
+
+/*
+ * Function pointer that is given a pointer to some memory, and from
+ * the compiler's point of view, _might_ read it, or otherwise depend
+ * on its contents.
+ *
+ * In fact, this function pointer always points to no_op() above. But
+ * because the pointer itself is volatile-qualified, the compiler
+ * isn't allowed to optimise based on the assumption that that will
+ * always be the case. So it has to call through the function pointer
+ * anyway, on the basis that it _might_ have magically changed at run
+ * time into a pointer to some completely arbitrary function. And
+ * therefore it must also avoid optimising away any observable effect
+ * beforehand that a completely arbitrary function might depend on -
+ * such as the zeroing of our memory re3gion.
+ */
+static void (*const volatile maybe_read)(void *ptr, size_t size) = no_op;
+
 void smemclr(void *b, size_t n)
 {
-    volatile char *vp;
-
     if (b && n > 0) {
         /*
          * Zero out the memory.
@@ -25,18 +43,10 @@ void smemclr(void *b, size_t n)
         memset(b, 0, n);
 
         /*
-         * Perform a volatile access to the object, forcing the
-         * compiler to admit that the previous memset was important.
-         *
-         * This while loop should in practice run for zero iterations
-         * (since we know we just zeroed the object out), but in
-         * theory (as far as the compiler knows) it might range over
-         * the whole object. (If we had just written, say, '*vp =
-         * *vp;', a compiler could in principle have 'helpfully'
-         * optimised the memset into only zeroing out the first byte.
-         * This should be robust.)
+         * Call the above function pointer, which (for all the
+         * compiler knows) might check that we've really zeroed the
+         * memory.
          */
-        vp = b;
-        while (*vp) vp++;
+        maybe_read(b, n);
     }
 }

windows/CMakeLists.txt

@@ -27,9 +27,6 @@ add_platform_sources_to_library(utils
   utils/version.c
   utils/win_strerror.c
   winucs.c)
-if(NOT HAVE_SECUREZEROMEMORY)
-  add_platform_sources_to_library(utils ../utils/smemclr.c)
-endif()
 if(NOT HAVE_STRTOUMAX)
   add_platform_sources_to_library(utils utils/strtoumax.c)
 endif()