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

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

[Daiki Ueno]

[-- 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

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

[Bruno Haible]

Hi Daiki,

> 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.
> ...
> It's tempting to make this behavior enabled by default, but I worry that
> it may cause any performance drawback.

Correct. For sensitive data, often different algorithms need to be used.
Explicit clearing of memory, avoiding algorithms whose running time depends
on the data, and possibly more.

> The attached patch adds a set of variants that deal with that.

Instead of doubling the number of functions of this header file, how about
adding a flags argument to the functions?

  #define RF_BINARY    0x1
  #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);

This way, the public interface of this header file even shrinks to 2 functions.

Yes, this breaks source-code backward compatibility, but Gnulib policy allows
this [1], and the users will have an easy migration path: just add a zero
argument for the flags.

If you agree, I'd like to see two commits:
  1. the introduction of the flags and RF_BINARY,
  2. the RF_SENSITIVE flag.

Do you want me to code the first commit, or do you want to do it?

Bruno

[1] https://www.gnu.org/software/gnulib/manual/html_node/Steady-Development.html

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

[Daiki Ueno]

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

Bruno Haible <bruno@clisp.org> writes:

> If you agree, I'd like to see two commits:
>   1. the introduction of the flags and RF_BINARY,
>   2. the RF_SENSITIVE flag.
>
> Do you want me to code the first commit, or do you want to do it?

Sure, that would make things much simpler.  I'm attaching a patch along
these lines.

Regards,
-- 
Daiki Ueno

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-read-file-add-flags-to-modify-reading-behavior.patch --]
[-- Type: text/x-patch, Size: 9547 bytes --]

From 5a40a392b3b551cf92d4be022b4efd900f339e39 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 flags to modify reading behavior

* lib/read-file.h (RF_BINARY, RF_SENSITIVE): New define.
(fread_file, read_file): Take FLAGS argument.
(read_binary_file): Remove.
* lib/read-file.c (internal_fread_file): Take into account of
RF_SENSITIVE flag.
(internal_read_file): Take into account of RF_BINARY and
RF_SENSITIVE flags.
* 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               | 15 ++++++++
 lib/read-file.c         | 85 ++++++++++++++++++++++++++++-------------
 lib/read-file.h         | 10 +++--
 modules/read-file       |  1 +
 modules/read-file-tests |  1 +
 tests/test-read-file.c  | 17 +++++++--
 6 files changed, 97 insertions(+), 32 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 07d4d5124..d1ee01a54 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2020-05-26  Daiki Ueno  <ueno@gnu.org>
+
+	read-file: add flags to modify reading behavior
+	* lib/read-file.h (RF_BINARY, RF_SENSITIVE): New define.
+	(fread_file, read_file): Take FLAGS argument.
+	(read_binary_file): Remove.
+	* lib/read-file.c (internal_fread_file): Take into account of
+	RF_SENSITIVE flag.
+	(internal_read_file): Take into account of RF_BINARY and
+	RF_SENSITIVE flags.
+	* 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-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..d95aae168 100644
--- a/lib/read-file.c
+++ b/lib/read-file.c
@@ -31,16 +31,17 @@
 /* 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 char *
+internal_fread_file (FILE *stream, int flags, size_t *length)
 {
   char *buf = NULL;
   size_t alloc = BUFSIZ;
@@ -94,7 +95,12 @@ 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;
+
+                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 +112,7 @@ fread_file (FILE *stream, size_t *length)
 
         {
           char *new_buf;
+          size_t save_alloc = alloc;
 
           if (alloc == PTRDIFF_MAX)
             {
@@ -118,7 +125,21 @@ fread_file (FILE *stream, 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,14 +149,32 @@ fread_file (FILE *stream, size_t *length)
         }
       }
 
+    if (flags & RF_SENSITIVE)
+      explicit_bzero (buf, alloc);
+
     free (buf);
     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.
+
+   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, size_t *length)
+{
+  return internal_fread_file (stream, flags, length);
+}
+
 static char *
-internal_read_file (const char *filename, size_t *length, const char *mode)
+internal_read_file (const char *filename, int flags, size_t *length,
+		    const char *mode)
 {
   FILE *stream = fopen (filename, mode);
   char *out;
@@ -144,7 +183,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;
 
@@ -153,6 +192,8 @@ internal_read_file (const char *filename, size_t *length, const char *mode)
       if (out)
         {
           save_errno = errno;
+	  if (flags & RF_SENSITIVE)
+	    explicit_bzero (out, *length);
           free (out);
         }
       errno = save_errno;
@@ -167,22 +208,14 @@ internal_read_file (const char *filename, size_t *length, const char *mode)
    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");
-}
+   any), and NULL is returned.
 
-/* 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.  */
+   If the RF_BINARY flag is set in FLAGS, the file is opened in binary
+   mode.  If the RF_SENSITIVE flag is set in FLAGS, the memory buffer
+   internally allocated will be cleared upon failure.  */
 char *
-read_binary_file (const char *filename, size_t *length)
+read_file (const char *filename, int flags, size_t *length)
 {
-  return internal_read_file (filename, length, "rbe");
+  const char *mode = (flags & RF_BINARY) ? "rbe" : "re";
+  return internal_read_file (filename, flags, length, mode);
 }
diff --git a/lib/read-file.h b/lib/read-file.h
index bb28abd65..c2454ef68 100644
--- a/lib/read-file.h
+++ b/lib/read-file.h
@@ -24,10 +24,14 @@
 /* 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);
+/* Indicate that the file content contains sensitive information.  */
+#define RF_SENSITIVE 0x2
 
-extern char *read_binary_file (const char *filename, size_t * length);
+extern char *fread_file (FILE * stream, int flags, 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 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..154fb36b1 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));
+  ASSERT (!test_read_file (RF_SENSITIVE));
+  ASSERT (!test_read_file (RF_BINARY | RF_SENSITIVE));
+}
-- 
2.26.2

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

[Bruno Haible]

Hi Daiki,

> > If you agree, I'd like to see two commits:
> >   1. the introduction of the flags and RF_BINARY,
> >   2. the RF_SENSITIVE flag.
> >
> > Do you want me to code the first commit, or do you want to do it?
> 
> Sure, that would make things much simpler.  I'm attaching a patch along
> these lines.

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.

Bruno

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

[Daiki Ueno]

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

Bruno Haible <bruno@clisp.org> 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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-read-file-add-flags-to-modify-reading-behavior.patch --]
[-- Type: text/x-patch, Size: 6799 bytes --]

From 60608590e2b106708dd74fd31331567af5166d2e Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@gnu.org>
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  <ueno@gnu.org>
+
+	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  <ueno@gnu.org>
 
 	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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-read-file-add-RF_SENSITIVE-flag.patch --]
[-- Type: text/x-patch, Size: 6017 bytes --]

From 874faad5aa189203d659b345427ff80cfab9301b Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@gnu.org>
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  <ueno@gnu.org>
+
+	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  <ueno@gnu.org>
 
 	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


[-- Attachment #4: Type: text/plain, Size: 25 bytes --]


Regards,
-- 
Daiki Ueno

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

[Bruno Haible]

Hi Daiki,

> Here are the two separate commits for this.

The first one is nearly perfect. Only the NEWS entry should go into section
"User visible incompatible changes", not into section "Important general notes".

The second one:
  - In fread_file, around line 100, you use a realloc workaround that appears
    to be not as reliable as the malloc+free based workaround around line 135.
  - In read_file, please don't use tabs for indentation. We've switched to
    spaces-only indentation long ago.

Once this is fixed, please push.

Bruno

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

[Daiki Ueno]

Bruno Haible <bruno@clisp.org> writes:

> The first one is nearly perfect. Only the NEWS entry should go into section
> "User visible incompatible changes", not into section "Important general notes".
>
> The second one:
>   - In fread_file, around line 100, you use a realloc workaround that appears
>     to be not as reliable as the malloc+free based workaround around line 135.
>   - In read_file, please don't use tabs for indentation. We've switched to
>     spaces-only indentation long ago.
>
> Once this is fixed, please push.

Both has been fixed and pushed.  Thank you for the review!

Regards,
-- 
Daiki Ueno

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

[Bruno Haible]

> Both has been fixed and pushed.  Thank you for the review!

Let me update the uses of the module 'read-file' in Gnulib.
I think the next weekly CI run would have caught this.


2020-05-28  Bruno Haible  <bruno@clisp.org>

	Fix build errors due to read-file changes (regression from 2020-05-27).
	* lib/git-merge-changelog.c (read_changelog_file): Update read_file
	invocation.
	* tests/test-sameacls.c (main): Likewise.
	* tests/test-pipe-filter-gi1.c (main): Call read_file instead of
	read_binary_file.
	* tests/test-pipe-filter-ii1.c (main): Likewise.

diff --git a/lib/git-merge-changelog.c b/lib/git-merge-changelog.c
index 1e6dae1..7b74a49 100644
--- a/lib/git-merge-changelog.c
+++ b/lib/git-merge-changelog.c
@@ -1,5 +1,5 @@
 /* git-merge-changelog - git "merge" driver for GNU style ChangeLog files.
-   Copyright (C) 2008-2010 Bruno Haible <bruno@clisp.org>
+   Copyright (C) 2008-2020 Bruno Haible <bruno@clisp.org>
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -300,7 +300,7 @@ read_changelog_file (const char *filename, struct changelog_file *result)
   /* Read the file in text mode, otherwise it's hard to recognize empty
      lines.  */
   size_t length;
-  char *contents = read_file (filename, &length);
+  char *contents = read_file (filename, 0, &length);
   if (contents == NULL)
     {
       fprintf (stderr, "could not read file '%s'\n", filename);
diff --git a/tests/test-pipe-filter-gi1.c b/tests/test-pipe-filter-gi1.c
index 4ee9375..0994610 100644
--- a/tests/test-pipe-filter-gi1.c
+++ b/tests/test-pipe-filter-gi1.c
@@ -80,7 +80,7 @@ main (int argc, char *argv[])
 
   /* Read some text from a file.  */
   input_filename = argv[2];
-  input = read_binary_file (input_filename, &input_size);
+  input = read_file (input_filename, RF_BINARY, &input_size);
   ASSERT (input != NULL);
 
   /* Convert it to uppercase, line by line.  */
diff --git a/tests/test-pipe-filter-ii1.c b/tests/test-pipe-filter-ii1.c
index 5f31d37..5a56c55 100644
--- a/tests/test-pipe-filter-ii1.c
+++ b/tests/test-pipe-filter-ii1.c
@@ -102,7 +102,7 @@ main (int argc, char *argv[])
 
   /* Read some text from a file.  */
   input_filename = argv[2];
-  input = read_binary_file (input_filename, &input_size);
+  input = read_file (input_filename, RF_BINARY, &input_size);
   ASSERT (input != NULL);
 
   /* Convert it to uppercase, line by line.  */
diff --git a/tests/test-sameacls.c b/tests/test-sameacls.c
index cdb10f4..6aad92f 100644
--- a/tests/test-sameacls.c
+++ b/tests/test-sameacls.c
@@ -55,14 +55,14 @@ main (int argc, char *argv[])
     size_t size2;
     char *contents2;
 
-    contents1 = read_file (file1, &size1);
+    contents1 = read_file (file1, 0, &size1);
     if (contents1 == NULL)
       {
         fprintf (stderr, "error reading file %s: errno = %d\n", file1, errno);
         fflush (stderr);
         abort ();
       }
-    contents2 = read_file (file2, &size2);
+    contents2 = read_file (file2, 0, &size2);
     if (contents2 == NULL)
       {
         fprintf (stderr, "error reading file %s: errno = %d\n", file2, errno);

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

[Daiki Ueno]

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

Bruno Haible <bruno@clisp.org> writes:

> Let me update the uses of the module 'read-file' in Gnulib.
> I think the next weekly CI run would have caught this.

Thank you; I completely missed those uses in Gnulib.

On a different note, it was suggested to disable stdio buffering if
RF_SENSITIVE is set.  I am attaching a patch for this.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-read-file-disable-buffering-if-RF_SENSITIVE-is-set.patch --]
[-- Type: text/x-patch, Size: 1177 bytes --]

From 9165e495461db91b8abc42661fc543784d26d0d6 Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@gnu.org>
Date: Fri, 29 May 2020 05:45:40 +0200
Subject: [PATCH] read-file: disable buffering if RF_SENSITIVE is set

* lib/read-file.c (read_file): Call setvbuf if RF_SENSITIVE.
Suggested by Glenn Strauss.
---
 ChangeLog       | 6 ++++++
 lib/read-file.c | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 77c637414..0a0e2301a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2020-05-29  Daiki Ueno  <ueno@gnu.org>
+
+	read-file: disable buffering if RF_SENSITIVE is set
+	* lib/read-file.c (read_file): Call setvbuf if RF_SENSITIVE.
+	Suggested by Glenn Strauss.
+
 2020-05-29  Daiki Ueno  <ueno@gnu.org>
 
 	fopen-gnu-tests: fix "\x" escape usage
diff --git a/lib/read-file.c b/lib/read-file.c
index 36780cc15..3520cbb7b 100644
--- a/lib/read-file.c
+++ b/lib/read-file.c
@@ -195,6 +195,9 @@ read_file (const char *filename, int flags, size_t *length)
   if (!stream)
     return NULL;
 
+  if (flags & RF_SENSITIVE)
+    setvbuf (stream, NULL, _IONBF, 0);
+
   out = fread_file (stream, flags, length);
 
   save_errno = errno;
-- 
2.26.2


[-- Attachment #3: Type: text/plain, Size: 25 bytes --]


Regards,
-- 
Daiki Ueno

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

[Bruno Haible]

Hi Daiki,

> On a different note, it was suggested to disable stdio buffering if
> RF_SENSITIVE is set.  I am attaching a patch for this.

Reading from a regular file in an unbuffered way can be terribly slow.
But here, fread_file reads in large chunks, therefore it's OK.

Also, in the specification of fread_file, I would add a note. Maybe like
this?

diff --git a/lib/read-file.c b/lib/read-file.c
index 36780cc..f13c528 100644
--- a/lib/read-file.c
+++ b/lib/read-file.c
@@ -43,8 +43,11 @@
    *LENGTH.  On errors, *LENGTH is undefined, errno preserves the
    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.  */
+   If the RF_SENSITIVE flag is set in FLAGS:
+     - You should control the buffering of STREAM using 'setvbuf'.  Either
+       clear the buffer of STREAM after closing it, or disable buffering of
+       STREAM before calling this function.
+     - The memory buffer internally allocated will be cleared upon failure.  */
 char *
 fread_file (FILE *stream, int flags, size_t *length)
 {

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

[Daiki Ueno]

Bruno Haible <bruno@clisp.org> writes:

>> On a different note, it was suggested to disable stdio buffering if
>> RF_SENSITIVE is set.  I am attaching a patch for this.
>
> Reading from a regular file in an unbuffered way can be terribly slow.
> But here, fread_file reads in large chunks, therefore it's OK.
>
> Also, in the specification of fread_file, I would add a note. Maybe like
> this?

Thank you, amended the documentation and pushed.

Regards,
-- 
Daiki Ueno