clamp snprintf result before fwrite in mtr_flush_with_state - #1185
clamp snprintf result before fwrite in mtr_flush_with_state#1185aysha-afrah26 wants to merge 1 commit into
Conversation
|
any update? |
There was a problem hiding this comment.
Verified on exact head 30ee5b3447b137b4b2d207b4f773f5f81bd6e24d with an independent ASan oracle compiled directly against minitrace.cpp. The same 1,200-byte event name makes exact base c88a9f429a421b312599a07fa8902524b09bf90a abort with a stack-buffer-overflow read of 1,280 bytes from the 1,024-byte linebuf at mtr_flush_with_state() / fwrite; exact head exits 0 and caps the trace output instead. The clamp also keeps a negative formatter return from becoming a huge unsigned write length. git diff --check passes and all 15 current upstream checks are green.
AI-assisted review performed with OpenAI Codex under fallenmi direction; the exact base/head oracle and live PR state were independently verified before submission.
MinitraceLogger passes node.name().c_str() to minitrace as the event name, and mtr_flush_with_state formats each event into a 1024 byte stack buffer before writing it out. The length handed to fwrite is snprintf's return value, which is the size the line would have needed rather than the size it actually wrote, so any event that overflows the buffer makes fwrite read past the end of linebuf. Node names come from the name attribute in the tree XML and nothing bounds their length, and mtr_flush() runs on every status transition, so a name of about a thousand characters is enough to reach it. The bytes read past the buffer are adjacent stack memory and they land verbatim in the trace file. I hit it under AddressSanitizer while looking at the logger tests: it reports a stack-buffer-overflow read of 1081 bytes out of the 1024 byte buffer at minitrace.cpp:356. Clamping len to what snprintf actually produced closes it, and treating a negative return as nothing-to-write also covers the MSVC _snprintf case where truncation reports -1 and fwrite would be called with (size_t)-1. I checked the rest of the tree for the same shape and this is the only place an snprintf return value is used as a length; the test added to gtest_loggers fails under ASan before the change and passes after.