Make ERR and thread use system malloc.
This will let us call ERR and thread_local from OPENSSL_malloc
without creating a circular dependency. We also make
ERR_get_error_line_data add ERR_FLAG_MALLOCED to the returned
flags value, since some projects appear to be making
assumptions about it being there.
Bug: 564
Update-Note: Any recent documentation (in all OpenSSL forks) for the ERR functions
cautions against freeing the returned ERR "data" strings, as freeing them is handled
by the error library. This change can make an existing double free bug more
obvious by being more likely to cause a crash with the double free.
Change-Id: Ie30bd3aee0b506473988b90675c48510969db31a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57045
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
diff --git a/crypto/err/err.c b/crypto/err/err.c
index 9b6d238..133a831 100644
--- a/crypto/err/err.c
+++ b/crypto/err/err.c
@@ -106,11 +106,15 @@
* (eay@cryptsoft.com). This product includes software written by Tim
* Hudson (tjh@cryptsoft.com). */
+// Ensure we can't call OPENSSL_malloc circularly.
+#define _BORINGSSL_PROHIBIT_OPENSSL_MALLOC
#include <openssl/err.h>
#include <assert.h>
#include <errno.h>
#include <inttypes.h>
+#include <limits.h>
+#include <stdarg.h>
#include <string.h>
#if defined(OPENSSL_WINDOWS)
@@ -129,8 +133,8 @@
struct err_error_st {
// file contains the filename where the error occurred.
const char *file;
- // data contains a NUL-terminated string with optional data. It must be freed
- // with |OPENSSL_free|.
+ // data contains a NUL-terminated string with optional data. It is allocated
+ // with system |malloc| and must be freed with |free| (not |OPENSSL_free|)
char *data;
// packed contains the error library and reason, as packed by ERR_PACK.
uint32_t packed;
@@ -162,7 +166,7 @@
// err_clear clears the given queued error.
static void err_clear(struct err_error_st *error) {
- OPENSSL_free(error->data);
+ free(error->data);
OPENSSL_memset(error, 0, sizeof(struct err_error_st));
}
@@ -170,12 +174,19 @@
err_clear(dst);
dst->file = src->file;
if (src->data != NULL) {
- dst->data = OPENSSL_strdup(src->data);
+ // Disable deprecated functions on msvc so it doesn't complain about strdup.
+ OPENSSL_MSVC_PRAGMA(warning(push))
+ OPENSSL_MSVC_PRAGMA(warning(disable : 4996))
+ // We can't use OPENSSL_strdup because we don't want to call OPENSSL_malloc,
+ // which can affect the error stack.
+ dst->data = strdup(src->data);
+ OPENSSL_MSVC_PRAGMA(warning(pop))
}
dst->packed = src->packed;
dst->line = src->line;
}
+
// global_next_library contains the next custom library value to return.
static int global_next_library = ERR_NUM_LIBS;
@@ -194,15 +205,15 @@
for (unsigned i = 0; i < ERR_NUM_ERRORS; i++) {
err_clear(&state->errors[i]);
}
- OPENSSL_free(state->to_free);
- OPENSSL_free(state);
+ free(state->to_free);
+ free(state);
}
// err_get_state gets the ERR_STATE object for the current thread.
static ERR_STATE *err_get_state(void) {
ERR_STATE *state = CRYPTO_get_thread_local(OPENSSL_THREAD_LOCAL_ERR);
if (state == NULL) {
- state = OPENSSL_malloc(sizeof(ERR_STATE));
+ state = malloc(sizeof(ERR_STATE));
if (state == NULL) {
return NULL;
}
@@ -258,7 +269,10 @@
} else {
*data = error->data;
if (flags != NULL) {
- *flags = ERR_FLAG_STRING;
+ // Without |ERR_FLAG_MALLOCED|, rust-openssl assumes the string has a
+ // static lifetime. In both cases, we retain ownership of the string,
+ // and the caller is not expected to free it.
+ *flags = ERR_FLAG_STRING | ERR_FLAG_MALLOCED;
}
// If this error is being removed, take ownership of data from
// the error. The semantics are such that the caller doesn't
@@ -267,7 +281,7 @@
// error queue.
if (inc) {
if (error->data != NULL) {
- OPENSSL_free(state->to_free);
+ free(state->to_free);
state->to_free = error->data;
}
error->data = NULL;
@@ -335,7 +349,7 @@
for (i = 0; i < ERR_NUM_ERRORS; i++) {
err_clear(&state->errors[i]);
}
- OPENSSL_free(state->to_free);
+ free(state->to_free);
state->to_free = NULL;
state->top = state->bottom = 0;
@@ -629,13 +643,13 @@
struct err_error_st *error;
if (state == NULL || state->top == state->bottom) {
- OPENSSL_free(data);
+ free(data);
return;
}
error = &state->errors[state->top];
- OPENSSL_free(error->data);
+ free(error->data);
error->data = data;
}
@@ -672,48 +686,42 @@
// concatenates them and sets the result as the data on the most recent
// error.
static void err_add_error_vdata(unsigned num, va_list args) {
- size_t alloced, new_len, len = 0, substr_len;
- char *buf;
+ size_t total_size = 0;
const char *substr;
- unsigned i;
+ char *buf;
- alloced = 80;
- buf = OPENSSL_malloc(alloced + 1);
- if (buf == NULL) {
+ va_list args_copy;
+ va_copy(args_copy, args);
+ for (size_t i = 0; i < num; i++) {
+ substr = va_arg(args_copy, const char *);
+ if (substr == NULL) {
+ continue;
+ }
+ size_t substr_len = strlen(substr);
+ if (SIZE_MAX - total_size < substr_len) {
+ return; // Would overflow.
+ }
+ total_size += substr_len;
+ }
+ va_end(args_copy);
+ if (total_size == SIZE_MAX) {
+ return; // Would overflow.
+ }
+ total_size += 1; // NUL terminator.
+ if ((buf = malloc(total_size)) == NULL) {
return;
}
-
- for (i = 0; i < num; i++) {
+ buf[0] = '\0';
+ for (size_t i = 0; i < num; i++) {
substr = va_arg(args, const char *);
if (substr == NULL) {
continue;
}
-
- substr_len = strlen(substr);
- new_len = len + substr_len;
- if (new_len > alloced) {
- char *new_buf;
-
- if (alloced + 20 + 1 < alloced) {
- // overflow.
- OPENSSL_free(buf);
- return;
- }
-
- alloced = new_len + 20;
- new_buf = OPENSSL_realloc(buf, alloced + 1);
- if (new_buf == NULL) {
- OPENSSL_free(buf);
- return;
- }
- buf = new_buf;
+ if (OPENSSL_strlcat(buf, substr, total_size) >= total_size) {
+ assert(0); // should not be possible.
}
-
- OPENSSL_memcpy(buf + len, substr, substr_len);
- len = new_len;
}
-
- buf[len] = 0;
+ va_end(args);
err_set_error_data(buf);
}
@@ -725,21 +733,13 @@
}
void ERR_add_error_dataf(const char *format, ...) {
+ char *buf = NULL;
va_list ap;
- char *buf;
- static const unsigned buf_len = 256;
-
- // A fixed-size buffer is used because va_copy (which would be needed in
- // order to call vsnprintf twice and measure the buffer) wasn't defined until
- // C99.
- buf = OPENSSL_malloc(buf_len + 1);
- if (buf == NULL) {
- return;
- }
va_start(ap, format);
- BIO_vsnprintf(buf, buf_len, format, ap);
- buf[buf_len] = 0;
+ if (OPENSSL_vasprintf_internal(&buf, format, ap, /*system_malloc=*/1) == -1) {
+ return;
+ }
va_end(ap);
err_set_error_data(buf);
@@ -751,13 +751,20 @@
assert(0);
return;
}
+ // Disable deprecated functions on msvc so it doesn't complain about strdup.
+ OPENSSL_MSVC_PRAGMA(warning(push))
+ OPENSSL_MSVC_PRAGMA(warning(disable : 4996))
+ // We can not use OPENSSL_strdup because we don't want to call OPENSSL_malloc,
+ // which can affect the error stack.
+ char *copy = strdup(data);
+ OPENSSL_MSVC_PRAGMA(warning(pop))
+ if (copy != NULL) {
+ err_set_error_data(copy);
+ }
if (flags & ERR_FLAG_MALLOCED) {
- err_set_error_data(data);
- } else {
- char *copy = OPENSSL_strdup(data);
- if (copy != NULL) {
- err_set_error_data(copy);
- }
+ // We can not take ownership of |data| directly because it is allocated with
+ // |OPENSSL_malloc| and we will free it with system |free| later.
+ OPENSSL_free(data);
}
}
@@ -819,8 +826,8 @@
for (size_t i = 0; i < state->num_errors; i++) {
err_clear(&state->errors[i]);
}
- OPENSSL_free(state->errors);
- OPENSSL_free(state);
+ free(state->errors);
+ free(state);
}
ERR_SAVE_STATE *ERR_save_state(void) {
@@ -829,7 +836,7 @@
return NULL;
}
- ERR_SAVE_STATE *ret = OPENSSL_malloc(sizeof(ERR_SAVE_STATE));
+ ERR_SAVE_STATE *ret = malloc(sizeof(ERR_SAVE_STATE));
if (ret == NULL) {
return NULL;
}
@@ -839,9 +846,9 @@
? state->top - state->bottom
: ERR_NUM_ERRORS + state->top - state->bottom;
assert(num_errors < ERR_NUM_ERRORS);
- ret->errors = OPENSSL_malloc(num_errors * sizeof(struct err_error_st));
+ ret->errors = malloc(num_errors * sizeof(struct err_error_st));
if (ret->errors == NULL) {
- OPENSSL_free(ret);
+ free(ret);
return NULL;
}
OPENSSL_memset(ret->errors, 0, num_errors * sizeof(struct err_error_st));
