Skip to content

clamp snprintf result before fwrite in mtr_flush_with_state - #1185

Open
aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:minitrace-snprintf-len
Open

clamp snprintf result before fwrite in mtr_flush_with_state#1185
aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:minitrace-snprintf-len

Conversation

@aysha-afrah26

Copy link
Copy Markdown
Contributor

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.

@aysha-afrah26

Copy link
Copy Markdown
Contributor Author

any update?

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants