[PATCH] read-file: add variants that clear internal memory

[Daiki Ueno <ueno@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

Hello,

The functions provided by the read-file module are handy, but they are
suboptimal for reading sensitive materials, because they do not clear
the allocated memory blocks upon failure.  The attached patch adds
a set of variants that deal with that.

It's tempting to make this behavior enabled by default, but I worry that
it may cause any performance drawback.

Regards,
-- 
Daiki Ueno

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-read-file-add-variants-that-clear-internal-memory.patch --]
[-- Type: text/x-patch, Size: 9118 bytes --]

From d2fc964fa1dd87f5970f28c22349fb6983ff379e Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@gnu.org>
Date: Tue, 26 May 2020 10:22:37 +0200
Subject: [PATCH] read-file: add variants that clear internal memory

* lib/read-file.h: Add declarations for fread_file_clear,
read_file_clear, and read_binary_file_clear.
* lib/read-file.c (clear_free, clear_realloc, fast_free)
(fast_realloc): New internal functions.
(internal_fread_file): Take free and realloc substitutes.
(internal_read_file): Take fread_file substitute.
(fread_file_clear, read_file_clear, read_binary_file_clear): New
functions.
* modules/read-file (Depends-on): Add explicit_bzero.
This adds a variant of those functions to explicitly
clear the internal memory block when it becomes unused.
---
 ChangeLog               |  15 ++++++
 lib/read-file.c         | 111 +++++++++++++++++++++++++++++++++++-----
 lib/read-file.h         |   6 +++
 modules/read-file       |   1 +
 modules/read-file-tests |   1 +
 tests/test-read-file.c  |  15 ++++--
 6 files changed, 132 insertions(+), 17 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 07d4d5124..c451ff057 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2020-05-26  Daiki Ueno  <ueno@gnu.org>
+
+	read-file: add variants that clear internal memory
+	* lib/read-file.h: Add declarations for fread_file_clear,
+	read_file_clear, and read_binary_file_clear.
+	* lib/read-file.c (clear_free, clear_realloc, fast_free)
+	(fast_realloc): New internal functions.
+	(internal_fread_file): Take free and realloc substitutes.
+	(internal_read_file): Take fread_file substitute.
+	(fread_file_clear, read_file_clear, read_binary_file_clear): New
+	functions.
+	* modules/read-file (Depends-on): Add explicit_bzero.
+	This adds a variant of those functions to explicitly
+	clear the internal memory block when it becomes unused.
+
 2020-05-26  Daiki Ueno  <ueno@gnu.org>
 
 	read-file: make use of fopen-gnu
diff --git a/lib/read-file.c b/lib/read-file.c
index 293bc3e8a..3fd94b046 100644
--- a/lib/read-file.c
+++ b/lib/read-file.c
@@ -31,16 +31,62 @@
 /* Get malloc, realloc, free. */
 #include <stdlib.h>
 
+/* Get explicit_bzero, memcpy. */
+#include <string.h>
+
 /* Get errno. */
 #include <errno.h>
 
-/* Read a STREAM and return a newly allocated string with the content,
-   and set *LENGTH to the length of the string.  The string is
-   zero-terminated, but the terminating zero byte is not counted in
-   *LENGTH.  On errors, *LENGTH is undefined, errno preserves the
-   values set by system functions (if any), and NULL is returned.  */
-char *
-fread_file (FILE *stream, size_t *length)
+/* Get assert. */
+#include <assert.h>
+
+static void
+clear_free (void *ptr, size_t old_size)
+{
+  if (ptr)
+    {
+      explicit_bzero (ptr, old_size);
+      free (ptr);
+    }
+}
+
+static void *
+clear_realloc (void *ptr, size_t old_size, size_t new_size)
+{
+  void *new_ptr;
+
+  assert (ptr);
+  assert (new_size);
+
+  if (new_size < old_size)
+    {
+      explicit_bzero (ptr + new_size, old_size - new_size);
+      return ptr;
+    }
+
+  new_ptr = malloc (new_size);
+  memcpy (new_ptr, ptr, old_size);
+  clear_free (ptr, old_size);
+  return new_ptr;
+}
+
+static void
+fast_free (void *ptr, size_t old_size _GL_UNUSED)
+{
+  if (ptr)
+    free (ptr);
+}
+
+static void *
+fast_realloc (void *ptr, size_t old_size _GL_UNUSED, size_t new_size)
+{
+  return realloc (ptr, new_size);
+}
+
+static char *
+internal_fread_file (FILE *stream, size_t *length,
+		     void (*free_func)(void *, size_t),
+		     void *(*realloc_func)(void *, size_t, size_t))
 {
   char *buf = NULL;
   size_t alloc = BUFSIZ;
@@ -94,7 +140,7 @@ fread_file (FILE *stream, size_t *length)
             /* Shrink the allocated memory if possible.  */
             if (size < alloc - 1)
               {
-                char *smaller_buf = realloc (buf, size + 1);
+                char *smaller_buf = realloc_func (buf, alloc, size + 1);
                 if (smaller_buf != NULL)
                   buf = smaller_buf;
               }
@@ -106,6 +152,7 @@ fread_file (FILE *stream, size_t *length)
 
         {
           char *new_buf;
+	  size_t save_alloc = alloc;
 
           if (alloc == PTRDIFF_MAX)
             {
@@ -118,7 +165,7 @@ fread_file (FILE *stream, size_t *length)
           else
             alloc = PTRDIFF_MAX;
 
-          if (!(new_buf = realloc (buf, alloc)))
+          if (!(new_buf = realloc_func (buf, save_alloc, alloc)))
             {
               save_errno = errno;
               break;
@@ -128,14 +175,34 @@ fread_file (FILE *stream, size_t *length)
         }
       }
 
-    free (buf);
+    free_func (buf, alloc);
     errno = save_errno;
     return NULL;
   }
 }
 
+/* Read a STREAM and return a newly allocated string with the content,
+   and set *LENGTH to the length of the string.  The string is
+   zero-terminated, but the terminating zero byte is not counted in
+   *LENGTH.  On errors, *LENGTH is undefined, errno preserves the
+   values set by system functions (if any), and NULL is returned.  */
+char *
+fread_file (FILE *stream, size_t *length)
+{
+  return internal_fread_file (stream, length, fast_free, fast_realloc);
+}
+
+/* Similar to fread_file, but clears any intermediate memory block
+   allocated internally.  */
+char *
+fread_file_clear (FILE *stream, size_t *length)
+{
+  return internal_fread_file (stream, length, clear_free, clear_realloc);
+}
+
 static char *
-internal_read_file (const char *filename, size_t *length, const char *mode)
+internal_read_file (const char *filename, size_t *length, const char *mode,
+		    char *(fread_file_func) (FILE *, size_t *))
 {
   FILE *stream = fopen (filename, mode);
   char *out;
@@ -144,7 +211,7 @@ internal_read_file (const char *filename, size_t *length, const char *mode)
   if (!stream)
     return NULL;
 
-  out = fread_file (stream, length);
+  out = fread_file_func (stream, length);
 
   save_errno = errno;
 
@@ -171,7 +238,15 @@ internal_read_file (const char *filename, size_t *length, const char *mode)
 char *
 read_file (const char *filename, size_t *length)
 {
-  return internal_read_file (filename, length, "re");
+  return internal_read_file (filename, length, "re", fread_file);
+}
+
+/* Similar to read_file, but clears any intermediate memory block
+   allocated internally.  */
+char *
+read_file_clear (const char *filename, size_t *length)
+{
+  return internal_read_file (filename, length, "re", fread_file_clear);
 }
 
 /* Open (on non-POSIX systems, in binary mode) and read the contents
@@ -184,5 +259,13 @@ read_file (const char *filename, size_t *length)
 char *
 read_binary_file (const char *filename, size_t *length)
 {
-  return internal_read_file (filename, length, "rbe");
+  return internal_read_file (filename, length, "rbe", fread_file);
+}
+
+/* Similar to read_binary_file, but clears any intermediate memory
+   block allocated internally.  */
+char *
+read_binary_file_clear (const char *filename, size_t *length)
+{
+  return internal_read_file (filename, length, "rbe", fread_file_clear);
 }
diff --git a/lib/read-file.h b/lib/read-file.h
index bb28abd65..a30e03acd 100644
--- a/lib/read-file.h
+++ b/lib/read-file.h
@@ -30,4 +30,10 @@ extern char *read_file (const char *filename, size_t * length);
 
 extern char *read_binary_file (const char *filename, size_t * length);
 
+extern char *fread_file_clear (FILE *stream, size_t *length);
+
+extern char *read_file_clear (const char *filename, size_t * length);
+
+extern char *read_binary_file_clear (const char *filename, size_t * length);
+
 #endif /* READ_FILE_H */
diff --git a/modules/read-file b/modules/read-file
index a6e7faf0a..5d2be5bbf 100644
--- a/modules/read-file
+++ b/modules/read-file
@@ -7,6 +7,7 @@ lib/read-file.c
 m4/read-file.m4
 
 Depends-on:
+explicit_bzero
 fopen-gnu
 fstat
 ftello
diff --git a/modules/read-file-tests b/modules/read-file-tests
index 361bca806..299631644 100644
--- a/modules/read-file-tests
+++ b/modules/read-file-tests
@@ -1,5 +1,6 @@
 Files:
 tests/test-read-file.c
+tests/macros.h
 
 Depends-on:
 
diff --git a/tests/test-read-file.c b/tests/test-read-file.c
index 930cf4acb..4b44cf2cd 100644
--- a/tests/test-read-file.c
+++ b/tests/test-read-file.c
@@ -23,11 +23,13 @@
 #include <stdlib.h>
 #include <sys/stat.h>
 
+#include "macros.h"
+
 #define FILE1 "/etc/resolv.conf"
 #define FILE2 "/dev/null"
 
 int
-main (void)
+test_read_file (char *(*read_file_func) (const char *, size_t *))
 {
   struct stat statbuf;
   int err = 0;
@@ -37,7 +39,7 @@ main (void)
   if (stat (FILE1, &statbuf) >= 0)
     {
       size_t len;
-      char *out = read_file (FILE1, &len);
+      char *out = read_file_func (FILE1, &len);
 
       if (!out)
         {
@@ -80,7 +82,7 @@ main (void)
   if (stat (FILE2, &statbuf) >= 0)
     {
       size_t len;
-      char *out = read_file (FILE2, &len);
+      char *out = read_file_func (FILE2, &len);
 
       if (!out)
         {
@@ -109,3 +111,10 @@ main (void)
 
   return err;
 }
+
+int
+main (void)
+{
+  ASSERT (!test_read_file (read_file));
+  ASSERT (!test_read_file (read_file_clear));
+}
-- 
2.26.2