From 10f8b36874fca928c3f41834babac8ee94dd3f09 Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Thu, 25 Jul 2019 17:51:30 +0200 Subject: [PATCH] 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 (Merged from https://github.com/openssl/openssl/pull/9459) --- crypto/err/err.c | 106 +++++++++++++++++++++++++++++------------- include/openssl/err.h | 1 + 2 files changed, 75 insertions(+), 32 deletions(-) diff --git a/crypto/err/err.c b/crypto/err/err.c index 71b1049a5d..7a35512f87 100644 --- 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 = ""; 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 3fa30ab2c1..e84bc68a4e 100644 --- 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]; -- 2.25.1