From e144b57b53e3af9d7ec1a09e4794081362ac5566 Mon Sep 17 00:00:00 2001 From: Stephan Hadinger Date: Thu, 17 Nov 2022 20:29:31 +0100 Subject: [PATCH] Avoid crash if malloc fails --- lib/default/Ext-printf/src/ext_printf.cpp | 31 +++++++++++------------ 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/default/Ext-printf/src/ext_printf.cpp b/lib/default/Ext-printf/src/ext_printf.cpp index 17a32701f..4ae6ee25e 100644 --- a/lib/default/Ext-printf/src/ext_printf.cpp +++ b/lib/default/Ext-printf/src/ext_printf.cpp @@ -208,9 +208,12 @@ char* ToHex_P(const unsigned char * in, size_t insz, char * out, size_t outsz, c // get a fresh malloc allocated string based on the current pointer (can be in PROGMEM) // It is the caller's responsibility to free the memory +// +// Returns nullptr if something went wrong char * copyStr(const char * str) { if (str == nullptr) { return nullptr; } char * cpy = (char*) malloc(strlen_P(str) + 1); + if (cpy == nullptr) { return nullptr; } // something went wrong strcpy_P(cpy, str); return cpy; } @@ -222,10 +225,13 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l va_list va_cpy; va_copy(va_cpy, va); + if (out_buf != nullptr) { *out_buf = '\0'; } // safeguard, if we abort, the result is an empty string + // iterate on fmt to extract arguments and patch them in place char * fmt_cpy = copyStr(fmt_P); - if (fmt_cpy == nullptr) { return 0; } + if (fmt_cpy == nullptr) { return 0; } // we couldn't copy the format, abort char * fmt = fmt_cpy; + int32_t ret = 0; // return 0 if unsuccessful const uint32_t ALLOC_SIZE = 12; static const char * allocs[ALLOC_SIZE] = {}; // initialized to zeroes @@ -277,6 +283,7 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l if (cur_val < min_valid_ptr) { new_val_str = ext_invalid_mem; } else if (decimals > 0) { char * hex_char = (char*) malloc(decimals*2 + 2); + if (hex_char == nullptr) { goto abort; } ToHex_P((const uint8_t *)cur_val, decimals, hex_char, decimals*2 + 2); new_val_str = hex_char; allocs[alloc_idx++] = new_val_str; @@ -292,6 +299,7 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l size_t buf_len = (&buf != nullptr) ? buf.len() : 0; if (buf_len) { char * hex_char = (char*) malloc(buf_len*2 + 2); + if (hex_char == nullptr) { goto abort; } ToHex_P(buf.getBuffer(), buf_len, hex_char, buf_len*2 + 2); new_val_str = hex_char; allocs[alloc_idx++] = new_val_str; @@ -307,6 +315,7 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l else if (decimals > 0) { uint32_t val_size = decimals*6 + 2; char * val_char = (char*) malloc(val_size); + if (val_char == nullptr) { goto abort; } val_char[0] = '\0'; for (uint32_t count = 0; count < decimals; count++) { uint32_t value = pgm_read_byte((const uint8_t *)cur_val +1) << 8 | pgm_read_byte((const uint8_t *)cur_val); @@ -328,6 +337,7 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l case 'I': // Input is `uint32_t` 32 bits IP address, output is decimal dotted address { char * ip_str = (char*) malloc(16); + if (ip_str == nullptr) { goto abort; } snprintf_P(ip_str, 16, PSTR("%u.%u.%u.%u"), cur_val & 0xFF, (cur_val >> 8) & 0xFF, (cur_val >> 16) & 0xFF, (cur_val >> 24) & 0xFF); new_val_str = ip_str; allocs[alloc_idx++] = new_val_str; @@ -371,6 +381,7 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l } } new_val_str = copyStr(hex); + if (new_val_str == nullptr) { goto abort; } allocs[alloc_idx++] = new_val_str; } } @@ -384,24 +395,11 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l if ((decimals < 0) || (decimals > 16)) { decimals = 16; } U64toHex(*(uint64_t*)cur_val, hex, decimals); new_val_str = copyStr(hex); + if (new_val_str == nullptr) { goto abort; } allocs[alloc_idx++] = new_val_str; } } break; - // Trying to do String allocation alternatives, but not as interesting as I thought in the beginning - // case 's': - // { - // new_val_str = copyStr(((String*)cur_val)->c_str()); - // allocs[alloc_idx++] = new_val_str; - // } - // break; - // case 'S': - // { - // funcString_t * func_str = (funcString_t*) cur_val; - // new_val_str = copyStr((*func_str)().c_str()); - // allocs[alloc_idx++] = new_val_str; - // } - // break; } *cur_val_ptr = new_val_str; *fmt = 's'; // replace `%_X` with `%0s` to display a string instead @@ -412,8 +410,8 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l } } } + // Serial.printf("> format_final=%s\n", fmt_cpy); Serial.flush(); - int32_t ret = 0; // return 0 if unsuccessful if (out_buf != nullptr) { ret = vsnprintf_P(out_buf, buf_len, fmt_cpy, va_cpy); } else { @@ -434,6 +432,7 @@ int32_t ext_vsnprintf_P(char * out_buf, size_t buf_len, const char * fmt_P, va_l va_end(va_cpy); +abort: // disallocated all temporary strings for (uint32_t i = 0; i < alloc_idx; i++) { free((void*)allocs[i]); // it is ok to call free() on nullptr so we don't test for nullptr first