ERR: re-use the err_data field when possible

To deallocate the err_data field and then allocating it again might be
a waste of processing, but may also be a source of errors when memory
is scarce.  While we normally tolerate that, the ERR sub-system is an
exception and we need to pay closer attention to how we handle memory.

This adds a new err_data flag, ERR_TXT_IGNORE, which means that even
if there is err_data memory allocated, its contents should be ignored.
Deallocation of the err_data field is much more selective, aand should
only happen when ERR_free_state() is called.

Fixes #9458

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/9459)

diff --git a/crypto/err/err.c b/crypto/err/err.c
index 71b1049a5db055d99e499be3b5c1a980eaaa4873..7a35512f879802ea08b18a1f3258f94856b32085 100644 (file)
--- a/crypto/err/err.c
+++ b/crypto/err/err.c
@@ -235,18 +235,34 @@ static void build_SYS_str_reasons(void)
 }
 #endif
 
-#define err_clear_data(p, i) \
-        do { \
-            if ((p)->err_data_flags[i] & ERR_TXT_MALLOCED) {\
-                OPENSSL_free((p)->err_data[i]); \
-                (p)->err_data[i] = NULL; \
-            } \
-            (p)->err_data_flags[i] = 0; \
+#define err_get_slot(p) \
+    do { \
+        (p)->top = ((p)->top + 1) % ERR_NUM_ERRORS; \
+        if ((p)->top == (p)->bottom) \
+            (p)->bottom = ((p)->bottom + 1) % ERR_NUM_ERRORS; \
+    } while (0)
+
+#define err_clear_data(p, i, deall)                             \
+        do {                                                    \
+            if ((p)->err_data_flags[i] & ERR_TXT_MALLOCED) {    \
+                if (deall) {                                    \
+                    OPENSSL_free((p)->err_data[i]);             \
+                    (p)->err_data[i] = NULL;                    \
+                    (p)->err_data_size[i] = 0;                  \
+                    (p)->err_data_flags[i] = 0;                 \
+                } else if ((p)->err_data[i] != NULL) {          \
+                    (p)->err_data[i][0] = '\0';                 \
+                }                                               \
+            } else {                                            \
+                (p)->err_data[i] = NULL;                        \
+                (p)->err_data_size[i] = 0;                      \
+                (p)->err_data_flags[i] = 0;                     \
+            }                                                   \
         } while (0)
 
-#define err_clear(p, i) \
+#define err_clear(p, i, deall) \
         do { \
-            err_clear_data(p, i); \
+            err_clear_data((p), (i), (deall)); \
             (p)->err_flags[i] = 0; \
             (p)->err_buffer[i] = 0; \
             (p)->err_file[i] = NULL; \
@@ -260,7 +276,7 @@ static void ERR_STATE_free(ERR_STATE *s)
     if (s == NULL)
         return;
     for (i = 0; i < ERR_NUM_ERRORS; i++) {
-        err_clear_data(s, i);
+        err_clear_data(s, i, 1);
     }
     OPENSSL_free(s);
 }
@@ -406,14 +422,11 @@ void ERR_put_error(int lib, int func, int reason, const char *file, int line)
     if (es == NULL)
         return;
 
-    es->top = (es->top + 1) % ERR_NUM_ERRORS;
-    if (es->top == es->bottom)
-        es->bottom = (es->bottom + 1) % ERR_NUM_ERRORS;
-    es->err_flags[es->top] = 0;
+    err_get_slot(es);
+    err_clear(es, es->top, 0);
     es->err_buffer[es->top] = ERR_PACK(lib, func, reason);
     es->err_file[es->top] = file;
     es->err_line[es->top] = line;
-    err_clear_data(es, es->top);
 }
 
 void ERR_clear_error(void)
@@ -426,7 +439,7 @@ void ERR_clear_error(void)
         return;
 
     for (i = 0; i < ERR_NUM_ERRORS; i++) {
-        err_clear(es, i);
+        err_clear(es, i, 0);
     }
     es->top = es->bottom = 0;
 }
@@ -506,14 +519,14 @@ static unsigned long get_error_values(int inc, int top, const char **file,
 
     while (es->bottom != es->top) {
         if (es->err_flags[es->top] & ERR_FLAG_CLEAR) {
-            err_clear(es, es->top);
+            err_clear(es, es->top, 0);
             es->top = es->top > 0 ? es->top - 1 : ERR_NUM_ERRORS - 1;
             continue;
         }
         i = (es->bottom + 1) % ERR_NUM_ERRORS;
         if (es->err_flags[i] & ERR_FLAG_CLEAR) {
             es->bottom = i;
-            err_clear(es, es->bottom);
+            err_clear(es, es->bottom, 0);
             continue;
         }
         break;
@@ -545,7 +558,7 @@ static unsigned long get_error_values(int inc, int top, const char **file,
 
     if (data == NULL) {
         if (inc) {
-            err_clear_data(es, i);
+            err_clear_data(es, i, 0);
         }
     } else {
         if (es->err_data[i] == NULL) {
@@ -772,7 +785,8 @@ int ERR_get_next_error_library(void)
     return ret;
 }
 
-static int err_set_error_data_int(char *data, int flags)
+static int err_set_error_data_int(char *data, size_t size, int flags,
+                                  int deallocate)
 {
     ERR_STATE *es;
     int i;
@@ -783,8 +797,9 @@ static int err_set_error_data_int(char *data, int flags)
 
     i = es->top;
 
-    err_clear_data(es, i);
+    err_clear_data(es, es->top, deallocate);
     es->err_data[i] = data;
+    es->err_data_size[i] = size;
     es->err_data_flags[i] = flags;
 
     return 1;
@@ -795,8 +810,18 @@ void ERR_set_error_data(char *data, int flags)
     /*
      * This function is void so we cannot propagate the error return. Since it
      * is also in the public API we can't change the return type.
+     *
+     * We estimate the size of the data.  If it's not flagged as allocated,
+     * then this is safe, and if it is flagged as allocated, then our size
+     * may be smaller than the actual allocation, but that doesn't matter
+     * too much, the buffer will remain untouched or will eventually be
+     * reallocated to a new size.
+     *
+     * callers should be advised that this function takes over ownership of
+     * the allocated memory, i.e. they can't count on the pointer to remain
+     * valid.
      */
-    err_set_error_data_int(data, flags);
+    err_set_error_data_int(data, strlen(data) + 1, flags, 1);
 }
 
 void ERR_add_error_data(int num, ...)
@@ -810,7 +835,8 @@ void ERR_add_error_data(int num, ...)
 void ERR_add_error_vdata(int num, va_list args)
 {
     int i, len, size;
-    char *str, *p, *arg;
+    int flags = ERR_TXT_MALLOCED | ERR_TXT_STRING;
+    char *str, *arg;
     ERR_STATE *es;
 
     /* Get the current error data; if an allocated string get it. */
@@ -818,16 +844,30 @@ void ERR_add_error_vdata(int num, va_list args)
     if (es == NULL)
         return;
     i = es->top;
-    p = es->err_data_flags[i] == (ERR_TXT_MALLOCED | ERR_TXT_STRING)
-            ? es->err_data[i] : "";
 
-    /* Start with initial (or empty) string and allocate a new buffer */
-    size = 80 + strlen(p);
-    if ((str = OPENSSL_malloc(size + 1)) == NULL) {
-        /* ERRerr(ERR_F_ERR_ADD_ERROR_VDATA, ERR_R_MALLOC_FAILURE); */
+    /*
+     * If err_data is allocated already, re-use the space.
+     * Otherwise, allocate a small new buffer.
+     */
+    if ((es->err_data_flags[i] & flags) == flags) {
+        str = es->err_data[i];
+        size = es->err_data_size[i];
+
+        /*
+         * To protect the string we just grabbed from tampering by other
+         * functions we may call, or to protect them from freeing a pointer
+         * that may no longer be valid at that point, we clear away the
+         * data pointer and the flags.  We will set them again at the end
+         * of this function.
+         */
+        es->err_data[i] = NULL;
+        es->err_data_flags[i] = 0;
+    } else if ((str = OPENSSL_malloc(size = 81)) == NULL) {
         return;
+    } else {
+        str[0] = '\0';
     }
-    strcpy(str, p);
+    len = strlen(str);
 
     for (len = 0; --num >= 0; ) {
         arg = va_arg(args, char *);
@@ -835,6 +875,8 @@ void ERR_add_error_vdata(int num, va_list args)
             arg = "<NULL>";
         len += strlen(arg);
         if (len > size) {
+            char *p;
+
             size = len + 20;
             p = OPENSSL_realloc(str, size + 1);
             if (p == NULL) {
@@ -845,7 +887,7 @@ void ERR_add_error_vdata(int num, va_list args)
         }
         OPENSSL_strlcat(str, arg, (size_t)size + 1);
     }
-    if (!err_set_error_data_int(str, ERR_TXT_MALLOCED | ERR_TXT_STRING))
+    if (!err_set_error_data_int(str, size, flags, 0))
         OPENSSL_free(str);
 }
 
@@ -873,7 +915,7 @@ int ERR_pop_to_mark(void)
 
     while (es->bottom != es->top
            && (es->err_flags[es->top] & ERR_FLAG_MARK) == 0) {
-        err_clear(es, es->top);
+        err_clear(es, es->top, 0);
         es->top = es->top > 0 ? es->top - 1 : ERR_NUM_ERRORS - 1;
     }
 

diff --git a/include/openssl/err.h b/include/openssl/err.h
index 3fa30ab2c19ece45eb455fbfe14dd8393ba93bb2..e84bc68a4ea542da67f76fbff209360377f8a362 100644 (file)
--- a/include/openssl/err.h
+++ b/include/openssl/err.h
@@ -46,6 +46,7 @@ typedef struct err_state_st {
     int err_flags[ERR_NUM_ERRORS];
     unsigned long err_buffer[ERR_NUM_ERRORS];
     char *err_data[ERR_NUM_ERRORS];
+    size_t err_data_size[ERR_NUM_ERRORS];
     int err_data_flags[ERR_NUM_ERRORS];
     const char *err_file[ERR_NUM_ERRORS];
     int err_line[ERR_NUM_ERRORS];