From c9665d0449ca5302dde91442351f5722d56354dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20K=C4=85dzio=C5=82ka?= Date: Sat, 27 Feb 2021 19:33:31 +0100 Subject: [PATCH 1/2] value_to_string: use snprintf Currently, value_to_string and debugger_value_to_string use an error-prone calculation to avoid overflow. This was once adjusted already, and one of the codepaths is still vulnerable. Put this in a symfile: 01:5678 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa and execute `p 1:$5679`. On Linux, the canary terminates the process. --- Core/debugger.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/Core/debugger.c b/Core/debugger.c index f6b4e4f..1b1ae29 100644 --- a/Core/debugger.c +++ b/Core/debugger.c @@ -131,30 +131,25 @@ static const char *value_to_string(GB_gameboy_t *gb, uint16_t value, bool prefer symbol = NULL; } - /* Avoid overflow */ - if (symbol && strlen(symbol->name) >= 240) { - symbol = NULL; - } - if (!symbol) { - sprintf(output, "$%04x", value); + snprintf(output, sizeof output, "$%04x", value); } else if (symbol->addr == value) { if (prefer_name) { - sprintf(output, "%s ($%04x)", symbol->name, value); + snprintf(output, sizeof output, "%s ($%04x)", symbol->name, value); } else { - sprintf(output, "$%04x (%s)", value, symbol->name); + snprintf(output, sizeof output, "$%04x (%s)", value, symbol->name); } } else { if (prefer_name) { - sprintf(output, "%s+$%03x ($%04x)", symbol->name, value - symbol->addr, value); + snprintf(output, sizeof output, "%s+$%03x ($%04x)", symbol->name, value - symbol->addr, value); } else { - sprintf(output, "$%04x (%s+$%03x)", value, symbol->name, value - symbol->addr); + snprintf(output, sizeof output, "$%04x (%s+$%03x)", value, symbol->name, value - symbol->addr); } } return output; @@ -171,30 +166,25 @@ static const char *debugger_value_to_string(GB_gameboy_t *gb, value_t value, boo symbol = NULL; } - /* Avoid overflow */ - if (symbol && strlen(symbol->name) >= 240) { - symbol = NULL; - } - if (!symbol) { - sprintf(output, "$%02x:$%04x", value.bank, value.value); + snprintf(output, sizeof output, "$%02x:$%04x", value.bank, value.value); } else if (symbol->addr == value.value) { if (prefer_name) { - sprintf(output, "%s ($%02x:$%04x)", symbol->name, value.bank, value.value); + snprintf(output, sizeof output, "%s ($%02x:$%04x)", symbol->name, value.bank, value.value); } else { - sprintf(output, "$%02x:$%04x (%s)", value.bank, value.value, symbol->name); + snprintf(output, sizeof output, "$%02x:$%04x (%s)", value.bank, value.value, symbol->name); } } else { if (prefer_name) { - sprintf(output, "%s+$%03x ($%02x:$%04x)", symbol->name, value.value - symbol->addr, value.bank, value.value); + snprintf(output, sizeof output, "%s+$%03x ($%02x:$%04x)", symbol->name, value.value - symbol->addr, value.bank, value.value); } else { - sprintf(output, "$%02x:$%04x (%s+$%03x)", value.bank, value.value, symbol->name, value.value - symbol->addr); + snprintf(output, sizeof output, "$%02x:$%04x (%s+$%03x)", value.bank, value.value, symbol->name, value.value - symbol->addr); } } return output; From 81bfea9ba246cf29a519f756da53a45497538e44 Mon Sep 17 00:00:00 2001 From: Lior Halphon Date: Sun, 28 Feb 2021 15:23:14 +0200 Subject: [PATCH 2/2] Coding style, ensuring string termination. --- Core/debugger.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/Core/debugger.c b/Core/debugger.c index 1b1ae29..0debf36 100644 --- a/Core/debugger.c +++ b/Core/debugger.c @@ -125,6 +125,7 @@ static inline void switch_banking_state(GB_gameboy_t *gb, uint16_t bank) static const char *value_to_string(GB_gameboy_t *gb, uint16_t value, bool prefer_name) { static __thread char output[256]; + output[sizeof(output) - 1] = 0; // Ensure termination const GB_bank_symbol_t *symbol = GB_debugger_find_symbol(gb, value); if (symbol && (value - symbol->addr > 0x1000 || symbol->addr == 0) ) { @@ -132,24 +133,24 @@ static const char *value_to_string(GB_gameboy_t *gb, uint16_t value, bool prefer } if (!symbol) { - snprintf(output, sizeof output, "$%04x", value); + snprintf(output, sizeof(output) - 1, "$%04x", value); } else if (symbol->addr == value) { if (prefer_name) { - snprintf(output, sizeof output, "%s ($%04x)", symbol->name, value); + snprintf(output, sizeof(output) - 1, "%s ($%04x)", symbol->name, value); } else { - snprintf(output, sizeof output, "$%04x (%s)", value, symbol->name); + snprintf(output, sizeof(output) - 1, "$%04x (%s)", value, symbol->name); } } else { if (prefer_name) { - snprintf(output, sizeof output, "%s+$%03x ($%04x)", symbol->name, value - symbol->addr, value); + snprintf(output, sizeof(output) - 1, "%s+$%03x ($%04x)", symbol->name, value - symbol->addr, value); } else { - snprintf(output, sizeof output, "$%04x (%s+$%03x)", value, symbol->name, value - symbol->addr); + snprintf(output, sizeof(output) - 1, "$%04x (%s+$%03x)", value, symbol->name, value - symbol->addr); } } return output; @@ -160,6 +161,7 @@ static const char *debugger_value_to_string(GB_gameboy_t *gb, value_t value, boo if (!value.has_bank) return value_to_string(gb, value.value, prefer_name); static __thread char output[256]; + output[sizeof(output) - 1] = 0; // Ensure termination const GB_bank_symbol_t *symbol = GB_map_find_symbol(gb->bank_symbols[value.bank], value.value); if (symbol && (value.value - symbol->addr > 0x1000 || symbol->addr == 0) ) { @@ -167,24 +169,24 @@ static const char *debugger_value_to_string(GB_gameboy_t *gb, value_t value, boo } if (!symbol) { - snprintf(output, sizeof output, "$%02x:$%04x", value.bank, value.value); + snprintf(output, sizeof(output) - 1, "$%02x:$%04x", value.bank, value.value); } else if (symbol->addr == value.value) { if (prefer_name) { - snprintf(output, sizeof output, "%s ($%02x:$%04x)", symbol->name, value.bank, value.value); + snprintf(output, sizeof(output) - 1, "%s ($%02x:$%04x)", symbol->name, value.bank, value.value); } else { - snprintf(output, sizeof output, "$%02x:$%04x (%s)", value.bank, value.value, symbol->name); + snprintf(output, sizeof(output) - 1, "$%02x:$%04x (%s)", value.bank, value.value, symbol->name); } } else { if (prefer_name) { - snprintf(output, sizeof output, "%s+$%03x ($%02x:$%04x)", symbol->name, value.value - symbol->addr, value.bank, value.value); + snprintf(output, sizeof(output) - 1, "%s+$%03x ($%02x:$%04x)", symbol->name, value.value - symbol->addr, value.bank, value.value); } else { - snprintf(output, sizeof output, "$%02x:$%04x (%s+$%03x)", value.bank, value.value, symbol->name, value.value - symbol->addr); + snprintf(output, sizeof(output) - 1, "$%02x:$%04x (%s+$%03x)", value.bank, value.value, symbol->name, value.value - symbol->addr); } } return output;