From 4212ff0b2869faee3366ea939f5f887a81af4cc6 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Tue, 28 Jul 2026 10:52:29 -0400 Subject: [PATCH] Fix format-string use of __FILE__/__func__ and name_for_status lower bound Two hardening fixes flagged by the earlier review: 1. FAIL passed __FILE__ and __func__ directly as the snprintf format string. __FILE__ expands to a string literal that could contain a '%' (a build path under a directory with a percent sign), and __func__ is not a literal at all; either way snprintf would read nonexistent varargs. Pass them as "%s" arguments instead. 2. akerr_name_for_status guarded the upper bound but not the lower one, so a negative status indexed __AKERR_ERROR_NAMES[negative] -- an out-of-bounds read, or an out-of-bounds write when a name was supplied. Reject status < 0. Regression tests err_format_string (uses #line to put a conversion specifier in __FILE__) and err_name_bounds fail against the old code (verified) and pass now. Full suite: 23/23, no warnings. Co-Authored-By: Claude Opus 4.8 (1M context) --- CMakeLists.txt | 2 ++ include/akerror.tmpl.h | 4 ++-- src/error.c | 2 +- tests/err_format_string.c | 34 ++++++++++++++++++++++++++++++++++ tests/err_name_bounds.c | 31 +++++++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 tests/err_format_string.c create mode 100644 tests/err_name_bounds.c diff --git a/CMakeLists.txt b/CMakeLists.txt index 13c0cee..02a9214 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -69,6 +69,8 @@ set(AKERR_TESTS err_maxval err_refcount_double_fail err_stacktrace_bounds + err_name_bounds + err_format_string ) set(AKERR_WILL_FAIL_TESTS diff --git a/include/akerror.tmpl.h b/include/akerror.tmpl.h index 78651f3..aa102ce 100644 --- a/include/akerror.tmpl.h +++ b/include/akerror.tmpl.h @@ -190,8 +190,8 @@ void akerr_init_errno(void); #define FAIL(__err_context, __err, __message, ...) \ ENSURE_ERROR_READY(__err_context); \ __err_context->status = __err; \ - snprintf((char *)__err_context->fname, AKERR_MAX_ERROR_FNAME_LENGTH, __FILE__); \ - snprintf((char *)__err_context->function, AKERR_MAX_ERROR_FUNCTION_LENGTH, __func__); \ + snprintf((char *)__err_context->fname, AKERR_MAX_ERROR_FNAME_LENGTH, "%s", __FILE__); \ + snprintf((char *)__err_context->function, AKERR_MAX_ERROR_FUNCTION_LENGTH, "%s", __func__); \ __err_context->lineno = __LINE__; \ snprintf((char *)__err_context->message, AKERR_MAX_ERROR_CONTEXT_STRING_LENGTH, __message, ## __VA_ARGS__); \ AKERR_STACKTRACE_APPEND(__err_context, "%s:%s:%d: %d (%s) : %s\n", (char *)__err_context->fname, (char *)__err_context->function, __err_context->lineno, __err_context->status, akerr_name_for_status(__err_context->status, NULL), (__err_context->message == NULL ? "" : __err_context->message)); diff --git a/src/error.c b/src/error.c index 17423b3..e31b731 100644 --- a/src/error.c +++ b/src/error.c @@ -118,7 +118,7 @@ akerr_ErrorContext *akerr_release_error(akerr_ErrorContext *err) // Call with name = NULL to retrieve a status. char *akerr_name_for_status(int status, char *name) { - if ( status > AKERR_MAX_ERR_VALUE ) { + if ( status < 0 || status > AKERR_MAX_ERR_VALUE ) { return "Unknown Error"; } if ( name != NULL ) { diff --git a/tests/err_format_string.c b/tests/err_format_string.c new file mode 100644 index 0000000..79d34c9 --- /dev/null +++ b/tests/err_format_string.c @@ -0,0 +1,34 @@ +#include "akerror.h" +#include "err_capture.h" +#include + +/* + * FAIL records the source file and function names with snprintf. Those names + * must be passed as %s ARGUMENTS, not used as the format string -- otherwise a + * path containing a printf conversion (say a build directory with a '%') is + * interpreted as a format and reads nonexistent varargs (undefined behavior). + * + * #line lets us make __FILE__ contain a conversion specifier; the stored name + * must come back verbatim. + */ + +#line 1 "pct%dname.c" +akerr_ErrorContext *raise_with_percent_in_filename(void) +{ + PREPARE_ERROR(e); + FAIL_RETURN(e, AKERR_VALUE, "boom"); +} +#line 22 "tests/err_format_string.c" + +int main(void) +{ + akerr_init(); + + akerr_ErrorContext *e = raise_with_percent_in_filename(); + AKERR_CHECK(e != NULL); + AKERR_CHECK(strcmp(e->fname, "pct%dname.c") == 0); + + e = akerr_release_error(e); + fprintf(stderr, "err_format_string ok\n"); + return 0; +} diff --git a/tests/err_name_bounds.c b/tests/err_name_bounds.c new file mode 100644 index 0000000..578ad71 --- /dev/null +++ b/tests/err_name_bounds.c @@ -0,0 +1,31 @@ +#include "akerror.h" +#include "err_capture.h" +#include + +/* + * akerr_name_for_status must reject out-of-range status codes at BOTH ends. + * It already guarded the upper bound, but a negative status indexed + * __AKERR_ERROR_NAMES[negative] -- an out-of-bounds read (and an out-of-bounds + * write when a name is supplied). Every out-of-range status must return the + * "Unknown Error" sentinel instead. + */ + +int main(void) +{ + akerr_init(); + + /* Below range. */ + AKERR_CHECK(strcmp(akerr_name_for_status(-1, NULL), "Unknown Error") == 0); + AKERR_CHECK(strcmp(akerr_name_for_status(-9999, NULL), "Unknown Error") == 0); + + /* Above range (already handled; kept as a guard against regressions). */ + AKERR_CHECK(strcmp(akerr_name_for_status(AKERR_MAX_ERR_VALUE + 1, NULL), + "Unknown Error") == 0); + + /* A valid code must still resolve to its real name. */ + AKERR_CHECK(strcmp(akerr_name_for_status(AKERR_NULLPOINTER, NULL), + "Null Pointer Error") == 0); + + fprintf(stderr, "err_name_bounds ok\n"); + return 0; +}