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

[Daiki Ueno]

Bruno Haible <address@hidden> writes:

> It would be useful to first concentrate on the first part, the refactoring
> that introduces flags and RF_BINARY. This would provide a patch that is easier
> to review and does not have the following problems:
>   - internal_fread_file still exists, although fread_file is a no-op wrapper
>     around it.
>   - In tests/test-read-file.c, please terminate the main() function with a
>     return statement. We assume C99 only in modules that explicitly list 'c99'
>     as a dependency. If it's trivial to avoid this dependency, let's do it.
>   - The NEWS file needs an entry.

Indeed, I missed the last paragraph of your suggestion in the previous
mail; sorry about that.  Here are the two separate commits for this.

>From 60608590e2b106708dd74fd31331567af5166d2e Mon Sep 17 00:00:00 2001
From: Daiki Ueno <address@hidden>
Date: Wed, 27 May 2020 08:14:44 +0200
Subject: [PATCH 1/2] read-file: add flags to modify reading behavior

* lib/read-file.h (RF_BINARY): New define.
(fread_file, read_file): Take FLAGS argument.
(read_binary_file): Remove.
* lib/read-file.c (internal_read_file): Merge into ...
(read_file): ... here.
* modules/read-file-tests (Files): Add "tests/macros.h".
* tests/test-read-file.c (main): Refactor using ASSERT macro.
* NEWS: Mention this change.
---
 ChangeLog               | 12 ++++++++++++
 NEWS                    |  5 +++++
 lib/read-file.c         | 43 ++++++++++++++---------------------------
 lib/read-file.h         |  7 ++++---
 modules/read-file-tests |  1 +
 tests/test-read-file.c  | 17 +++++++++++++---
 6 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 07d4d5124..94faf6984 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2020-05-27  Daiki Ueno  <address@hidden>
+
+       read-file: add flags to modify reading behavior
+       * lib/read-file.h (RF_BINARY): New define.
+       (fread_file, read_file): Take FLAGS argument.
+       (read_binary_file): Remove.
+       * lib/read-file.c (internal_read_file): Merge into ...
+       (read_file): ... here.
+       * modules/read-file-tests (Files): Add "tests/macros.h".
+       * tests/test-read-file.c (main): Refactor using ASSERT macro.
+       * NEWS: Mention this change.
+
 2020-05-26  Daiki Ueno  <address@hidden>
 
        read-file: make use of fopen-gnu
diff --git a/NEWS b/NEWS
index 99973c5c3..c559a65e9 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,11 @@ Important general notes
 
 Date        Modules         Changes
 
+2020-05-27  read-file       The functions provided by this module now take an
+                            'int flags' argument to modify the file reading
+                            behavior.  The read_binary_file function has been
+                            removed as it is no longer necessary.
+
 2019-12-11  Support for     These modules are now supported in C++ mode as 
well.
             ISO C or POSIX  This means, while the autoconfiguration uses the C
             functions       compiler, the resulting header files and function
diff --git a/lib/read-file.c b/lib/read-file.c
index 293bc3e8a..904f1c901 100644
--- a/lib/read-file.c
+++ b/lib/read-file.c
@@ -40,7 +40,7 @@
    *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)
+fread_file (FILE *stream, int flags _GL_UNUSED, size_t *length)
 {
   char *buf = NULL;
   size_t alloc = BUFSIZ;
@@ -134,9 +134,19 @@ fread_file (FILE *stream, size_t *length)
   }
 }
 
-static char *
-internal_read_file (const char *filename, size_t *length, const char *mode)
+/* Open and read the contents of FILENAME, 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.
+
+   If the RF_BINARY flag is set in FLAGS, the file is opened in binary
+   mode.  */
+char *
+read_file (const char *filename, int flags, size_t *length)
 {
+  const char *mode = (flags & RF_BINARY) ? "rbe" : "re";
   FILE *stream = fopen (filename, mode);
   char *out;
   int save_errno;
@@ -144,7 +154,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 (stream, flags, length);
 
   save_errno = errno;
 
@@ -161,28 +171,3 @@ internal_read_file (const char *filename, size_t *length, 
const char *mode)
 
   return out;
 }
-
-/* Open and read the contents of FILENAME, 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 *
-read_file (const char *filename, size_t *length)
-{
-  return internal_read_file (filename, length, "re");
-}
-
-/* Open (on non-POSIX systems, in binary mode) and read the contents
-   of FILENAME, 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
-   the LENGTH variable.  On errors, *LENGTH is undefined, errno
-   preserves the values set by system functions (if any), and NULL is
-   returned.  */
-char *
-read_binary_file (const char *filename, size_t *length)
-{
-  return internal_read_file (filename, length, "rbe");
-}
diff --git a/lib/read-file.h b/lib/read-file.h
index bb28abd65..7ff82ca77 100644
--- a/lib/read-file.h
+++ b/lib/read-file.h
@@ -24,10 +24,11 @@
 /* Get FILE.  */
 #include <stdio.h>
 
-extern char *fread_file (FILE * stream, size_t * length);
+/* Indicate that the file is treated as binary.  */
+#define RF_BINARY 0x1
 
-extern char *read_file (const char *filename, size_t * length);
+extern char *fread_file (FILE * stream, int flags, size_t * length);
 
-extern char *read_binary_file (const char *filename, size_t * length);
+extern char *read_file (const char *filename, int flags, size_t * length);
 
 #endif /* READ_FILE_H */
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..84b904994 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 (int flags)
 {
   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 (FILE1, flags, &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 (FILE2, flags, &len);
 
       if (!out)
         {
@@ -109,3 +111,12 @@ main (void)
 
   return err;
 }
+
+int
+main (void)
+{
+  ASSERT (!test_read_file (0));
+  ASSERT (!test_read_file (RF_BINARY));
+
+  return 0;
+}
-- 
2.26.2

>From 874faad5aa189203d659b345427ff80cfab9301b Mon Sep 17 00:00:00 2001
From: Daiki Ueno <address@hidden>
Date: Tue, 26 May 2020 10:22:37 +0200
Subject: [PATCH 2/2] read-file: add RF_SENSITIVE flag

* lib/read-file.h (RF_SENSITIVE): New define.
* lib/read-file.c (fread_file, read_file): Take into account of
RF_SENSITIVE flag.
* modules/read-file (Depends-on): Add explicit_bzero.
This adds an alternative behavior of those functions to explicitly
clear the internal memory block when it becomes unused.  This is
useful for reading sensitive information from a file.
---
 ChangeLog              | 11 +++++++++++
 lib/read-file.c        | 42 +++++++++++++++++++++++++++++++++++++-----
 lib/read-file.h        |  3 +++
 modules/read-file      |  1 +
 tests/test-read-file.c |  2 ++
 5 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 94faf6984..4a160faa6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2020-05-27  Daiki Ueno  <address@hidden>
+
+       read-file: add RF_SENSITIVE flag
+       * lib/read-file.h (RF_SENSITIVE): New define.
+       * lib/read-file.c (fread_file, read_file): Take into account of
+       RF_SENSITIVE flag.
+       * modules/read-file (Depends-on): Add explicit_bzero.
+       This adds an alternative behavior of those functions to explicitly
+       clear the internal memory block when it becomes unused.  This is
+       useful for reading sensitive information from a file.
+
 2020-05-27  Daiki Ueno  <address@hidden>
 
        read-file: add flags to modify reading behavior
diff --git a/lib/read-file.c b/lib/read-file.c
index 904f1c901..8bf3fdbe4 100644
--- a/lib/read-file.c
+++ b/lib/read-file.c
@@ -31,6 +31,9 @@
 /* Get malloc, realloc, free. */
 #include <stdlib.h>
 
+/* Get explicit_bzero, memcpy. */
+#include <string.h>
+
 /* Get errno. */
 #include <errno.h>
 
@@ -38,9 +41,12 @@
    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.  */
+   values set by system functions (if any), and NULL is returned.
+
+   If the RF_SENSITIVE flag is set in FLAGS, the memory buffer
+   internally allocated will be cleared upon failure.  */
 char *
-fread_file (FILE *stream, int flags _GL_UNUSED, size_t *length)
+fread_file (FILE *stream, int flags, size_t *length)
 {
   char *buf = NULL;
   size_t alloc = BUFSIZ;
@@ -94,7 +100,12 @@ fread_file (FILE *stream, int flags _GL_UNUSED, size_t 
*length)
             /* Shrink the allocated memory if possible.  */
             if (size < alloc - 1)
               {
-                char *smaller_buf = realloc (buf, size + 1);
+                char *smaller_buf;
+
+                if (flags & RF_SENSITIVE)
+                  explicit_bzero (buf + size, alloc - size);
+
+                smaller_buf = realloc (buf, size + 1);
                 if (smaller_buf != NULL)
                   buf = smaller_buf;
               }
@@ -106,6 +117,7 @@ fread_file (FILE *stream, int flags _GL_UNUSED, size_t 
*length)
 
         {
           char *new_buf;
+          size_t save_alloc = alloc;
 
           if (alloc == PTRDIFF_MAX)
             {
@@ -118,7 +130,21 @@ fread_file (FILE *stream, int flags _GL_UNUSED, size_t 
*length)
           else
             alloc = PTRDIFF_MAX;
 
-          if (!(new_buf = realloc (buf, alloc)))
+          if (flags & RF_SENSITIVE)
+            {
+              new_buf = malloc (alloc);
+              if (!new_buf)
+                {
+                  /* BUF should be cleared below after the loop.  */
+                  save_errno = errno;
+                  break;
+                }
+              memcpy (new_buf, buf, save_alloc);
+              explicit_bzero (buf, save_alloc);
+              free (buf);
+              buf = new_buf;
+            }
+          else if (!(new_buf = realloc (buf, alloc)))
             {
               save_errno = errno;
               break;
@@ -128,6 +154,9 @@ fread_file (FILE *stream, int flags _GL_UNUSED, size_t 
*length)
         }
       }
 
+    if (flags & RF_SENSITIVE)
+      explicit_bzero (buf, alloc);
+
     free (buf);
     errno = save_errno;
     return NULL;
@@ -142,7 +171,8 @@ fread_file (FILE *stream, int flags _GL_UNUSED, size_t 
*length)
    any), and NULL is returned.
 
    If the RF_BINARY flag is set in FLAGS, the file is opened in binary
-   mode.  */
+   mode.  If the RF_SENSITIVE flag is set in FLAGS, the memory buffer
+   internally allocated will be cleared upon failure.  */
 char *
 read_file (const char *filename, int flags, size_t *length)
 {
@@ -163,6 +193,8 @@ read_file (const char *filename, int flags, size_t *length)
       if (out)
         {
           save_errno = errno;
+         if (flags & RF_SENSITIVE)
+           explicit_bzero (out, *length);
           free (out);
         }
       errno = save_errno;
diff --git a/lib/read-file.h b/lib/read-file.h
index 7ff82ca77..c2454ef68 100644
--- a/lib/read-file.h
+++ b/lib/read-file.h
@@ -27,6 +27,9 @@
 /* Indicate that the file is treated as binary.  */
 #define RF_BINARY 0x1
 
+/* Indicate that the file content contains sensitive information.  */
+#define RF_SENSITIVE 0x2
+
 extern char *fread_file (FILE * stream, int flags, size_t * length);
 
 extern char *read_file (const char *filename, int flags, size_t * length);
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/tests/test-read-file.c b/tests/test-read-file.c
index 84b904994..b37f875b2 100644
--- a/tests/test-read-file.c
+++ b/tests/test-read-file.c
@@ -117,6 +117,8 @@ main (void)
 {
   ASSERT (!test_read_file (0));
   ASSERT (!test_read_file (RF_BINARY));
+  ASSERT (!test_read_file (RF_SENSITIVE));
+  ASSERT (!test_read_file (RF_BINARY | RF_SENSITIVE));
 
   return 0;
 }
-- 
2.26.2

Regards,
-- 
Daiki Ueno