Skip to content

clamp vsnprintf result before copying in log_printf #9398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Nashit-h wants to merge 1 commit into
base: main
Choose a base branch
from
+8 −0
Open

clamp vsnprintf result before copying in log_printf #9398

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/runtime/hexagon_remote/qurt/log.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ void log_printf(const char *fmt, ...) {
va_start(ap, fmt);
int message_size = vsnprintf(message, sizeof(message) - 1, fmt, ap);
va_end(ap);
if (message_size < 0) {
return;
}
// vsnprintf returns the length the message would have been, not the number
// of bytes written, so clamp to what actually fit before copying it out.
if (message_size > (int)sizeof(message) - 1) {
message_size = sizeof(message) - 1;
}

@mcourteaux mcourteaux Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The -1 is confusing here, because above vsnprintf() is already passing sizeof(message)-1 as bufsize. So according to doc:

Writes the results to a character string buffer. At most bufsz - 1 characters are written. The resulting character string will be terminated with a null character, unless bufsz is zero. If bufsz is zero, nothing is written and buffer may be a null pointer, however the return value (number of bytes that would be written not including the null terminator) is still calculated and returned.

So I think the -1 at line 67 is unnecessary and the one here is required. IIUC, the -1 here, actually MUST be a -2 given that the -1 in line 67 is there. So I propose to keep this -1 and instead remove the -1 from line 67.

@alexreinking alexreinking Aug 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, the docs are just a little unclear... vsnprintf(..., N, ...) writes N bytes, which is N - 1 characters plus a null terminator. So sizeof(message) - 1 is indeed the size of the message (in bytes), which is what write expects.

number of bytes that would be written not including the null terminator

However, the return value is not the message_size, but the message_length in characters. So this comparison needs to be >= not >.

@mcourteaux mcourteaux Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know. My analysis stands.

@alexreinking alexreinking Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No? - 2 would be off by 1 still IIUC

@mcourteaux mcourteaux Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

vsnprintf(..., N, ...) is already called with N=sizeof(buf) - 1 in the existing code. That's not a useful pattern: the -1 is not needed for correct behavior in vsnprintf to not write out of bounds. The correct approach would be:

    int message_size = vsnprintf(message, sizeof(message), fmt, ap);
    // message_size is ideal character count.
    va_end(ap);
    if (message_size < 0) {
        return;
    }
    // if more characters required than the buffer is long, limit ourselves to the buffer:
    if (message_size > (int)sizeof(message)) {
        message_size = sizeof(message);
    }
    global_log.write(message, message_size); // write non-null terminated character sequence

(So yes, I was wrong. -2 is not needed if the N = size - 1.)

global_log.write(message, message_size);
}

Expand Down
Loading