Fix iree_status_format capacity checks for partial buffer fills (#24797)
The message payload formatter wrote its NUL terminator one byte past the
end of the buffer when truncating and reported the truncated length
instead of the required length. The stack trace helpers underflowed the
remaining capacity after a truncated write, causing further
out-of-bounds writes. Payload formatters now follow snprintf-style
semantics: the full required length is always reported and writes never
exceed the given capacity. Adds StatusFormatBoundary tests covering
every buffer capacity with guard bytes.
Fixes #16874
Signed-off-by: Irfanuet191 <irfanhussainuet191@gmail.com>
diff --git a/runtime/src/iree/base/status.c b/runtime/src/iree/base/status.c
index f0141cd..7547df4 100644
--- a/runtime/src/iree/base/status.c
+++ b/runtime/src/iree/base/status.c
@@ -395,25 +395,22 @@
}
// Formats an iree_status_payload_message_t to the given output |buffer|.
-// |out_buffer_length| will be set to the number of characters written excluding
-// NUL. If |buffer| is omitted then |out_buffer_length| will be set to the
-// total number of characters in |buffer_capacity| required to contain the
-// entire message.
+// Follows snprintf-style semantics: |out_buffer_length| is always set to the
+// total number of characters required to contain the entire message excluding
+// NUL, even if |buffer_capacity| is insufficient. If |buffer| is provided then
+// as much of the message as fits is written NUL-terminated.
static void iree_status_payload_message_formatter(
const iree_status_payload_t* base_payload, iree_host_size_t buffer_capacity,
char* buffer, iree_host_size_t* out_buffer_length) {
iree_status_payload_message_t* payload =
(iree_status_payload_message_t*)base_payload;
- if (!buffer) {
- *out_buffer_length = payload->message.size;
- return;
- }
- iree_host_size_t n = buffer_capacity < payload->message.size
- ? buffer_capacity
+ *out_buffer_length = payload->message.size;
+ if (!buffer || buffer_capacity == 0) return;
+ iree_host_size_t n = buffer_capacity - 1 < payload->message.size
+ ? buffer_capacity - 1
: payload->message.size;
memcpy(buffer, payload->message.data, n);
buffer[n] = '\0';
- *out_buffer_length = n;
}
// Captures the current stack and attaches it to the status storage.
@@ -860,9 +857,9 @@
return true;
}
- // Prefix with source location and status code string (may be 'OK').
+ // Total length written (or that would have been written) excluding NUL.
iree_host_size_t buffer_length = 0;
- int n = 0;
+ int n IREE_ATTRIBUTE_UNUSED = 0;
#if (IREE_STATUS_FEATURES & IREE_STATUS_FEATURE_ANNOTATIONS) != 0
// Append base storage message.
@@ -879,14 +876,9 @@
buffer_length += n;
}
#endif // has IREE_STATUS_FEATURE_ANNOTATIONS
- if (IREE_UNLIKELY(n < 0)) {
- return false;
- } else if (buffer && n >= buffer_capacity) {
- buffer = NULL;
- }
#if IREE_STATUS_FEATURES != 0
- // Append each payload separated by a newline.
+ // Append each payload separated by '; '.
iree_status_payload_t* payload = storage ? storage->payload_head : NULL;
while (payload != NULL) {
// Skip payloads that have no textual representation.
@@ -895,18 +887,19 @@
continue;
}
- // Append newline to join with message above and other payloads.
- if (buffer) {
- if (2 >= buffer_capacity - buffer_length) {
- buffer = NULL;
- } else {
- buffer[buffer_length] = ';';
- buffer[buffer_length + 1] = ' ';
- }
+ // Append separator to join with message above and other payloads.
+ n = iree_snprintf(buffer ? buffer + buffer_length : NULL,
+ buffer ? buffer_capacity - buffer_length : 0, "; ");
+ if (IREE_UNLIKELY(n < 0)) {
+ return false;
+ } else if (buffer && n >= buffer_capacity - buffer_length) {
+ buffer = NULL;
}
- buffer_length += 2; // '; '
+ buffer_length += n;
- // Append payload via custom formatter callback.
+ // Append payload via custom formatter callback. Formatters have
+ // snprintf-style semantics: |payload_buffer_length| receives the length
+ // required for the entire payload even when truncated.
iree_host_size_t payload_buffer_length = 0;
payload->formatter(payload, buffer ? buffer_capacity - buffer_length : 0,
buffer ? buffer + buffer_length : NULL,
diff --git a/runtime/src/iree/base/status.h b/runtime/src/iree/base/status.h
index c10400b..fe6b30b 100644
--- a/runtime/src/iree/base/status.h
+++ b/runtime/src/iree/base/status.h
@@ -653,9 +653,11 @@
// two-pass pattern: call with buffer_capacity=0/buffer=NULL to query the
// required length, then call again with a sufficiently sized buffer.
//
-// Sets |*out_buffer_length| to the number of characters written (or required),
-// excluding NUL. Sets |*out_buffer_length| to 0 if the payload has no
-// formatter.
+// Follows snprintf-style semantics: sets |*out_buffer_length| to the number of
+// characters required to format the entire payload excluding NUL, even if
+// |buffer_capacity| is insufficient, and writes as much NUL-terminated content
+// as fits when |buffer| is provided. Sets |*out_buffer_length| to 0 if the
+// payload has no formatter.
IREE_API_EXPORT void iree_status_payload_format(
const iree_status_payload_t* payload, iree_host_size_t buffer_capacity,
char* buffer, iree_host_size_t* out_buffer_length);
diff --git a/runtime/src/iree/base/status_payload.h b/runtime/src/iree/base/status_payload.h
index 3bb0d1e..9d2f3cf 100644
--- a/runtime/src/iree/base/status_payload.h
+++ b/runtime/src/iree/base/status_payload.h
@@ -22,7 +22,13 @@
typedef struct iree_status_storage_t iree_status_storage_t;
-// Function that formats a payload into a human-readable string form for logs.
+// Formats a payload into a human-readable string with snprintf-style
+// semantics: |out_buffer_length| is always set to the total number of
+// characters required to format the entire payload excluding the NUL
+// terminator, even if |buffer_capacity| is insufficient. If |buffer| is
+// provided then as much of the payload as fits is written NUL-terminated;
+// implementations must never write at or beyond |buffer| + |buffer_capacity|.
+// If |buffer| is NULL then no characters are written (measurement pass).
typedef void(IREE_API_PTR* iree_status_payload_formatter_t)(
const iree_status_payload_t* payload, iree_host_size_t buffer_capacity,
char* buffer, iree_host_size_t* out_buffer_length);
diff --git a/runtime/src/iree/base/status_stack_trace.c b/runtime/src/iree/base/status_stack_trace.c
index ff94397..a7bf016 100644
--- a/runtime/src/iree/base/status_stack_trace.c
+++ b/runtime/src/iree/base/status_stack_trace.c
@@ -57,13 +57,20 @@
return iree_make_string_view(file_name, file_name_length);
}
+// Appends to |buffer| at offset |buffer_length| with snprintf-style semantics:
+// the returned length is always advanced by the length required for the entire
+// string even if the write was truncated (or |buffer_length| already exceeds
+// |buffer_capacity| from a previous truncated append), keeping the total
+// required length accounting intact while never writing out of bounds.
static iree_host_size_t iree_string_buffer_append_cstr(
iree_host_size_t buffer_capacity, char* buffer,
iree_host_size_t buffer_length, const char* str) {
- iree_host_size_t n =
- iree_snprintf(buffer ? buffer + buffer_length : NULL,
- buffer ? buffer_capacity - buffer_length : 0, "%s", str);
- return IREE_UNLIKELY(n < 0) ? 0 : buffer_length + n;
+ iree_host_size_t remaining = buffer && buffer_length < buffer_capacity
+ ? buffer_capacity - buffer_length
+ : 0;
+ int n = iree_snprintf(remaining ? buffer + buffer_length : NULL, remaining,
+ "%s", str);
+ return IREE_UNLIKELY(n < 0) ? buffer_length : buffer_length + n;
}
static iree_host_size_t IREE_PRINTF_ATTRIBUTE(4, 5)
@@ -71,13 +78,15 @@
char* buffer,
iree_host_size_t buffer_length,
const char* format, ...) {
+ iree_host_size_t remaining = buffer && buffer_length < buffer_capacity
+ ? buffer_capacity - buffer_length
+ : 0;
va_list varargs;
va_start(varargs, format);
- iree_host_size_t n = iree_vsnprintf(
- buffer ? buffer + buffer_length : NULL,
- buffer ? buffer_capacity - buffer_length : 0, format, varargs);
+ int n = iree_vsnprintf(remaining ? buffer + buffer_length : NULL, remaining,
+ format, varargs);
va_end(varargs);
- return IREE_UNLIKELY(n < 0) ? 0 : buffer_length + n;
+ return IREE_UNLIKELY(n < 0) ? buffer_length : buffer_length + n;
}
#if defined(IREE_USE_LIBBACKTRACE)
@@ -457,16 +466,18 @@
char* buffer, iree_host_size_t* out_buffer_length) {
iree_status_payload_stack_trace_t* payload =
(iree_status_payload_stack_trace_t*)base_payload;
- if (payload->frame_count - payload->skip_frames == 0) return;
+ *out_buffer_length = 0;
+ if (payload->frame_count <= payload->skip_frames) return;
iree_host_size_t buffer_length =
iree_string_buffer_append_cstr(buffer_capacity, buffer, 0, "stack:\n");
for (iree_host_size_t i = payload->skip_frames + 1; i < payload->frame_count;
++i) {
+ iree_host_size_t frame_capacity = buffer && buffer_length < buffer_capacity
+ ? buffer_capacity - buffer_length
+ : 0;
buffer_length += iree_status_payload_stack_trace_format_frame(
- (void*)payload->addresses[i],
- buffer ? buffer_capacity - buffer_length : 0,
- buffer ? buffer + buffer_length : NULL);
- if (buffer_length > buffer_capacity) buffer = NULL;
+ (void*)payload->addresses[i], frame_capacity,
+ frame_capacity ? buffer + buffer_length : NULL);
}
*out_buffer_length = buffer_length;
}
diff --git a/runtime/src/iree/base/status_test.cc b/runtime/src/iree/base/status_test.cc
index 3310509..a8c9349 100644
--- a/runtime/src/iree/base/status_test.cc
+++ b/runtime/src/iree/base/status_test.cc
@@ -4,6 +4,7 @@
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#include <cstring>
#include <ostream>
#include <string>
#include <type_traits>
@@ -166,6 +167,50 @@
EXPECT_THAT(cb_result, HasSubstr("INTERNAL"));
}
+// Helper: formats |status| into buffers of every capacity in
+// [0, full_length + 2] and verifies snprintf-style semantics at each boundary:
+// - |out_buffer_length| always reports the full required length.
+// - The buffer holds a NUL-terminated prefix of the full string.
+// - No bytes are written at or beyond the provided capacity.
+// Regression test for https://github.com/iree-org/iree/issues/16874.
+static void CheckFormatBoundaries(iree_status_t status) {
+ // Measure the full length (NULL buffer) and format the reference string.
+ iree_host_size_t full_length = 0;
+ ASSERT_TRUE(iree_status_format(status, 0, NULL, &full_length));
+ std::string full(full_length + 1, '\0');
+ iree_host_size_t verify_length = 0;
+ ASSERT_TRUE(iree_status_format(
+ status, full.size(), const_cast<char*>(full.data()), &verify_length));
+ ASSERT_EQ(verify_length, full_length);
+ full.resize(full_length);
+
+ constexpr char kGuard = '\xAB';
+ constexpr iree_host_size_t kGuardBytes = 8;
+ for (iree_host_size_t capacity = 0; capacity <= full_length + 2; ++capacity) {
+ // Guard bytes trailing the usable capacity detect out-of-bounds writes.
+ std::string buffer(capacity + kGuardBytes, kGuard);
+ iree_host_size_t out_length = 0;
+ ASSERT_TRUE(iree_status_format(
+ status, capacity, const_cast<char*>(buffer.data()), &out_length));
+ ASSERT_EQ(out_length, full_length) << "capacity=" << capacity;
+ ASSERT_EQ(buffer.substr(capacity), std::string(kGuardBytes, kGuard))
+ << "out-of-bounds write at capacity=" << capacity;
+ if (capacity == 0) continue;
+ const void* nul = memchr(buffer.data(), '\0', capacity);
+ ASSERT_NE(nul, nullptr) << "missing NUL at capacity=" << capacity;
+ iree_host_size_t written = (const char*)nul - buffer.data();
+ ASSERT_EQ(std::string(buffer.data(), written), full.substr(0, written))
+ << "not a prefix at capacity=" << capacity;
+ if (capacity > full_length) {
+ ASSERT_EQ(written, full_length) << "capacity=" << capacity;
+ }
+ }
+}
+
+TEST(StatusFormatBoundary, CodeOnly) {
+ CheckFormatBoundaries(iree_status_from_code(IREE_STATUS_DATA_LOSS));
+}
+
#if (IREE_STATUS_FEATURES & IREE_STATUS_FEATURE_ANNOTATIONS) != 0
TEST(StatusFormatTo, WithMessage) {
@@ -220,6 +265,22 @@
iree_status_free(status);
}
+TEST(StatusFormatBoundary, WithMessage) {
+ iree_status_t status = iree_status_allocate_f(
+ IREE_STATUS_INVALID_ARGUMENT, NULL, 0, "some message here %d", 42);
+ CheckFormatBoundaries(status);
+ iree_status_free(status);
+}
+
+TEST(StatusFormatBoundary, WithAnnotations) {
+ iree_status_t status =
+ iree_status_allocate_f(IREE_STATUS_NOT_FOUND, NULL, 0, "base message");
+ status = iree_status_annotate_f(status, "annotation one %s", "abc");
+ status = iree_status_annotate_f(status, "annotation two");
+ CheckFormatBoundaries(status);
+ iree_status_free(status);
+}
+
TEST(StatusFormatTo, CallbackShortCircuit) {
iree_status_t status =
iree_status_allocate_f(IREE_STATUS_INTERNAL, NULL, 0, "error %d", 42);
