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) <noreply@anthropic.com>
This commit is contained in:
@@ -69,6 +69,8 @@ set(AKERR_TESTS
|
|||||||
err_maxval
|
err_maxval
|
||||||
err_refcount_double_fail
|
err_refcount_double_fail
|
||||||
err_stacktrace_bounds
|
err_stacktrace_bounds
|
||||||
|
err_name_bounds
|
||||||
|
err_format_string
|
||||||
)
|
)
|
||||||
|
|
||||||
set(AKERR_WILL_FAIL_TESTS
|
set(AKERR_WILL_FAIL_TESTS
|
||||||
|
|||||||
@@ -190,8 +190,8 @@ void akerr_init_errno(void);
|
|||||||
#define FAIL(__err_context, __err, __message, ...) \
|
#define FAIL(__err_context, __err, __message, ...) \
|
||||||
ENSURE_ERROR_READY(__err_context); \
|
ENSURE_ERROR_READY(__err_context); \
|
||||||
__err_context->status = __err; \
|
__err_context->status = __err; \
|
||||||
snprintf((char *)__err_context->fname, AKERR_MAX_ERROR_FNAME_LENGTH, __FILE__); \
|
snprintf((char *)__err_context->fname, AKERR_MAX_ERROR_FNAME_LENGTH, "%s", __FILE__); \
|
||||||
snprintf((char *)__err_context->function, AKERR_MAX_ERROR_FUNCTION_LENGTH, __func__); \
|
snprintf((char *)__err_context->function, AKERR_MAX_ERROR_FUNCTION_LENGTH, "%s", __func__); \
|
||||||
__err_context->lineno = __LINE__; \
|
__err_context->lineno = __LINE__; \
|
||||||
snprintf((char *)__err_context->message, AKERR_MAX_ERROR_CONTEXT_STRING_LENGTH, __message, ## __VA_ARGS__); \
|
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));
|
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));
|
||||||
|
|||||||
@@ -118,7 +118,7 @@ akerr_ErrorContext *akerr_release_error(akerr_ErrorContext *err)
|
|||||||
// Call with name = NULL to retrieve a status.
|
// Call with name = NULL to retrieve a status.
|
||||||
char *akerr_name_for_status(int status, char *name)
|
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";
|
return "Unknown Error";
|
||||||
}
|
}
|
||||||
if ( name != NULL ) {
|
if ( name != NULL ) {
|
||||||
|
|||||||
34
tests/err_format_string.c
Normal file
34
tests/err_format_string.c
Normal file
@@ -0,0 +1,34 @@
|
|||||||
|
#include "akerror.h"
|
||||||
|
#include "err_capture.h"
|
||||||
|
#include <string.h>
|
||||||
|
|
||||||
|
/*
|
||||||
|
* 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;
|
||||||
|
}
|
||||||
31
tests/err_name_bounds.c
Normal file
31
tests/err_name_bounds.c
Normal file
@@ -0,0 +1,31 @@
|
|||||||
|
#include "akerror.h"
|
||||||
|
#include "err_capture.h"
|
||||||
|
#include <string.h>
|
||||||
|
|
||||||
|
/*
|
||||||
|
* 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;
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user