From a847bec8ace49bd33fa4bcd17cd5c2eac501fddc Mon Sep 17 00:00:00 2001 From: Kevin Newton Date: Sat, 24 Oct 2026 21:10:18 -0400 Subject: [PATCH] Avoid vsnprintf when possible When formatting error messages, first try to format into a fixed buffer instead of formatting once to get the length then a second time to get the actual format. --- templates/src/diagnostic.c.erb | 20 ++++++++++++++++---- test/prism/errors_test.rb | 15 +++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/templates/src/diagnostic.c.erb b/templates/src/diagnostic.c.erb index d8e7c1c289..7283411186 100644 --- a/templates/src/diagnostic.c.erb +++ b/templates/src/diagnostic.c.erb @@ -12,6 +12,7 @@ #include #include #include +#include #define PM_DIAGNOSTIC_ID_MAX <%= errors.length + warnings.length %> @@ -534,11 +535,16 @@ pm_diagnostic_list_append(pm_arena_t *arena, pm_list_t *list, uint32_t start, ui */ void pm_diagnostic_list_append_format(pm_arena_t *arena, pm_list_t *list, uint32_t start, uint32_t length, pm_diagnostic_id_t diag_id, ...) { + /* Format the message into a buffer on the stack, which nearly every message + * fits in. The result is the length of the whole message whether or not it + * fit, so a single pass both formats the message and sizes it. */ + char stack[256]; + va_list arguments; va_start(arguments, diag_id); const char *format = pm_diagnostic_id_message(diag_id); - int result = vsnprintf(NULL, 0, format, arguments); + int result = vsnprintf(stack, sizeof(stack), format, arguments); va_end(arguments); if (result < 0) { @@ -550,9 +556,15 @@ pm_diagnostic_list_append_format(pm_arena_t *arena, pm_list_t *list, uint32_t st size_t message_length = (size_t) (result + 1); char *message = (char *) pm_arena_alloc(arena, message_length, 1); - va_start(arguments, diag_id); - vsnprintf(message, message_length, format, arguments); - va_end(arguments); + if (message_length <= sizeof(stack)) { + memcpy(message, stack, message_length); + } else { + /* The message was cut off on the stack, so format it again into an + * allocation of the right size. */ + va_start(arguments, diag_id); + vsnprintf(message, message_length, format, arguments); + va_end(arguments); + } *diagnostic = (pm_diagnostic_t) { .location = { .start = start, .length = length }, diff --git a/test/prism/errors_test.rb b/test/prism/errors_test.rb index 9dd7fbe3fe..0637671ba4 100644 --- a/test/prism/errors_test.rb +++ b/test/prism/errors_test.rb @@ -83,6 +83,21 @@ def test_regexp_encoding_option_mismatch_error assert_empty result.errors end + def test_long_formatted_messages + # Messages are formatted into a fixed-size buffer first, so check lengths + # on either side of its size to cover both the copy and the reformat. + template = "unterminated heredoc; can't find string \"%s\" anywhere before EOF" + + (254..258).each do |length| + identifier = "E" * (length - template.length + 2) + expected = format(template, identifier) + assert_equal length, expected.bytesize + + messages = Prism.parse("<<#{identifier}\n").errors.map(&:message) + assert_includes messages, expected + end + end + def test_incomplete_def_closing_loc statement = Prism.parse_statement("def f; 123") assert_nil(statement.end_keyword)