clamp vsnprintf result before copying in log_printf

[Nashit-h]

1. vsnprintf returns the length the message would have been, not the number of bytes written into the 1024-byte stack buffer, so a formatted string longer than that leaves message_size pointing past the buffer.
2. global_log.write() then copies message[0..message_size-1] out of the buffer, and those bytes are handed back to the host through halide_hexagon_remote_poll_log.
   Clamp message_size to the bytes that actually fit (and bail on the negative error return) before the copy. log_printf backs halide_print/halide_error on the DSP, so an over-long printed or error string reaches it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.20%. Comparing base (347b0d1) to head (2357f2b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9398      +/-   ##
==========================================
+ Coverage   70.14%   70.20%   +0.05%     
==========================================
  Files         261      261              
  Lines       79388    79388              
  Branches    19357    19357              
==========================================
+ Hits        55690    55737      +47     
+ Misses      17874    17869       -5     
+ Partials     5824     5782      -42     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcourteaux]

Good catch, although I believe the fix is not 100% correct.

[mcourteaux]

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]

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]

I know. My analysis stands.

[alexreinking]

No? - 2 would be off by 1 still IIUC

[mcourteaux]

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.)