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
>From d2fc964fa1dd87f5970f28c22349fb6983ff379e Mon Sep 17 00:00:00 2001
From: Daiki Ueno <u...@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 <u...@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 <u...@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
