Retain ownership of malloced error data.

I misunderstood the OpenSSL semantics here. When receiving an error data
pointer via ERR_get_error_line_data and friends, although the error is
cleared, OpenSSL retains ownership of the data pointer. It's kept in the
cleared error until another error overrides it, or the whole error queue
is cleared.

It's pretty odd to have live pointers in empty errors so this change
allows an error queue to retain one data pointer. Thus the pointer
returned from ERR_get_error_line_data is valid until the next call to
ERR_get_error_line_data, or until the queue is freed.

From reviewing uses of the API, this is sufficient for all of them.

Change-Id: I73cb8e9c792452ae3c1a934ac8bbe8b5353b65b2
Reviewed-on: https://boringssl-review.googlesource.com/1880
Reviewed-by: Adam Langley <agl@google.com>

crypto/err/err.c

@@ -168,7 +168,7 @@ static ERR_STATE *err_get_state(void) {
 }
 
 static uint32_t get_error_values(int inc, int top, const char **file, int *line,
-                                 char **data, int *flags) {
+                                 const char **data, int *flags) {
   unsigned i = 0;
   ERR_STATE *state;
   struct err_error_st *error;
@@ -211,6 +211,12 @@ static uint32_t get_error_values(int inc, int top, const char **file, int *line,
       if (flags != NULL) {
         *flags = error->flags & ERR_FLAG_PUBLIC_MASK;
       }
+      if (error->flags & ERR_FLAG_MALLOCED) {
+        if (state->to_free) {
+          OPENSSL_free(state->to_free);
+        }
+        state->to_free = error->data;
+      }
       error->data = NULL;
       error->flags = 0;
     }
@@ -234,7 +240,7 @@ uint32_t ERR_get_error_line(const char **file, int *line) {
 }
 
 uint32_t ERR_get_error_line_data(const char **file, int *line,
-                                 char **data, int *flags) {
+                                 const char **data, int *flags) {
   return get_error_values(1, 0, file, line, data, flags);
 }
 
@@ -248,7 +254,7 @@ uint32_t ERR_peek_error_line(const char **file, int *line) {
 
 uint32_t ERR_peek_error_line_data(const char **file, int *line,
                                   const char **data, int *flags) {
-  return get_error_values(0, 0, file, line, (char **) data, flags);
+  return get_error_values(0, 0, file, line, data, flags);
 }
 
 uint32_t ERR_peek_last_error(void) {
@@ -261,7 +267,7 @@ uint32_t ERR_peek_last_error_line(const char **file, int *line) {
 
 uint32_t ERR_peek_last_error_line_data(const char **file, int *line,
                                        const char **data, int *flags) {
-  return get_error_values(0, 1, file, line, (char **) data, flags);
+  return get_error_values(0, 1, file, line, data, flags);
 }
 
 void ERR_clear_error(void) {
@@ -271,6 +277,9 @@ void ERR_clear_error(void) {
   for (i = 0; i < ERR_NUM_ERRORS; i++) {
     err_clear(&state->errors[i]);
   }
+  if (state->to_free) {
+    OPENSSL_free(state->to_free);
+  }
 
   state->top = state->bottom = 0;
 }
@@ -294,7 +303,9 @@ void ERR_remove_thread_state(const CRYPTO_THREADID *tid) {
   for (i = 0; i < ERR_NUM_ERRORS; i++) {
     err_clear(&state->errors[i]);
   }
-
+  if (state->to_free) {
+    OPENSSL_free(state->to_free);
+  }
   OPENSSL_free(state);
 }
 
@@ -433,8 +444,7 @@ void ERR_print_errors_cb(ERR_print_errors_callback_t callback, void *ctx) {
   char buf[ERR_ERROR_STRING_BUF_LEN];
   char buf2[1024];
   unsigned long thread_hash;
-  const char *file;
-  char *data;
+  const char *file, *data;
   int line, flags;
   uint32_t packed_error;
 
@@ -453,9 +463,6 @@ void ERR_print_errors_cb(ERR_print_errors_callback_t callback, void *ctx) {
     if (callback(buf2, strlen(buf2), ctx) <= 0) {
       break;
     }
-    if (flags & ERR_FLAG_MALLOCED) {
-      OPENSSL_free(data);
-    }
   }
 }
 

crypto/err/err_test.c

@@ -44,8 +44,7 @@ static int test_overflow(void) {
 static int test_put_error(void) {
   uint32_t packed_error;
   int line, flags;
-  const char *file;
-  char *data;
+  const char *file, *data;
 
   if (ERR_get_error() != 0) {
     fprintf(stderr, "ERR_get_error returned value before an error was added.\n");
@@ -68,8 +67,6 @@ static int test_put_error(void) {
     return 0;
   }
 
-  OPENSSL_free(data);
-
   return 1;
 }
 

include/openssl/err.h

@@ -166,9 +166,12 @@ OPENSSL_EXPORT uint32_t ERR_get_error_line(const char **file, int *line);
 
 /* ERR_get_error_line_data acts like |ERR_get_error_line|, but also returns the
  * error-specific data pointer and flags. The flags are a bitwise-OR of
- * |ERR_FLAG_*| values. */
+ * |ERR_FLAG_*| values. The error-specific data is owned by the error queue
+ * and the pointer becomes invalid after the next call that affects the same
+ * thread's error queue. If |*flags| contains |ERR_FLAG_STRING| then |*data| is
+ * human-readable. */
 OPENSSL_EXPORT uint32_t ERR_get_error_line_data(const char **file, int *line,
-                                                char **data, int *flags);
+                                                const char **data, int *flags);
 
 /* The "peek" functions act like the |ERR_get_error| functions, above, but they
  * do not remove the error from the queue. */
@@ -325,12 +328,9 @@ struct err_error_st {
   uint8_t flags;
 };
 
-/* ERR_FLAG_MALLOCED means the the |data| member must be freed when no longer
- * needed. */
-#define ERR_FLAG_MALLOCED 1
 /* ERR_FLAG_STRING means that the |data| member is a NUL-terminated string that
  * can be printed. */
-#define ERR_FLAG_STRING 2
+#define ERR_FLAG_STRING 1
 /* ERR_TXT_STRING is provided for compatibility with code that assumes that
  * it's using OpenSSL. */
 #define ERR_TXT_STRING ERR_FLAG_STRING
@@ -342,9 +342,12 @@ struct err_error_st {
 /* The following flag values are internal and are masked when flags are
  * returned from functions like |ERR_get_error_line_data|. */
 
+/* ERR_FLAG_MALLOCED means the the |data| member must be freed when no longer
+ * needed. */
+#define ERR_FLAG_MALLOCED 16
 /* ERR_FLAG_MARK is used to indicate a reversion point in the queue. See
  * |ERR_pop_to_mark|. */
-#define ERR_FLAG_MARK 16
+#define ERR_FLAG_MARK 32
 
 /* ERR_NUM_ERRORS is the limit of the number of errors in the queue. */
 #define ERR_NUM_ERRORS 16
@@ -362,6 +365,10 @@ typedef struct err_state_st {
   unsigned top;
   /* bottom contains the index of the last error in the queue. */
   unsigned bottom;
+
+  /* to_free, if not NULL, contains a pointer owned by this structure that was
+   * previously a |data| pointer of one of the elements of |errors|. */
+  void *to_free;
 } ERR_STATE;
 
 enum {