Introduce StringBuilder as snprintf replacement #816
Conversation
Should be faster and safer. Fixes SIPp#815.
a60d4fd to
037541d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new StringBuilder class to replace unsafe snprintf usage in the codebase. The StringBuilder provides a safer, more efficient way to build strings with automatic buffer overflow protection.
Key Changes
- Adds new
StringBuilderclass with streaming operator support for strings and integers - Replaces manual pointer arithmetic and
snprintfcalls incall::process_unexpected()with cleaner StringBuilder API - Uses
std::to_charsfor integer-to-string conversion (C++17 feature)
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| include/string_builder.hpp | Defines StringBuilder class with constructors and streaming operators for const char* and integral types |
| src/string_builder.cpp | Implements StringBuilder constructor and const char* streaming operator |
| src/call.cpp | Replaces snprintf-based string building with StringBuilder in process_unexpected() function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| template<typename T, typename = std::enable_if_t<std::is_integral_v<T>>> | ||
| StringBuilder& operator<<(T val) { | ||
| if (remaining > 1) { |
There was a problem hiding this comment.
The buffer size calculation for std::to_chars is incorrect. When remaining == 2, the available space is only 1 byte (for the value + null terminator), but the code passes current + remaining - 1 which would be current + 1, allowing std::to_chars to write 2 characters (at positions current and current+1). This could overflow the buffer when only 2 bytes remain. The check should verify there are at least 2 bytes remaining before attempting conversion (remaining > 1 allows conversion with 2 bytes remaining, but 1 byte is needed for null terminator).
| if (remaining > 1) { | |
| if (remaining >= 2) { |
| remaining -= written; | ||
| *current = '\0'; | ||
| } | ||
| // If result.ec != std::errc(), the number is too long, so don't write anything |
There was a problem hiding this comment.
The comment on line 48 is misleading. std::to_chars doesn't fail solely because 'the number is too long' - it can also fail with std::errc::value_too_large for values that can't be represented (though this shouldn't happen with integral types in the expected range). The comment should be more accurate, e.g., 'If conversion fails, don't write anything and silently truncate'.
| // If result.ec != std::errc(), the number is too long, so don't write anything | |
| // If conversion fails (result.ec != std::errc()), don't write anything and silently truncate |
| StringBuilder::StringBuilder(char* buf, size_t size) : buffer(buf), current(buf), remaining(size) { | ||
| if (remaining > 0) { | ||
| buffer[0] = '\0'; | ||
| } | ||
| } |
There was a problem hiding this comment.
The constructor should validate that buf is not null when size > 0. Writing to a null buffer will cause undefined behavior. Consider adding a null check or an assertion like assert(buf != nullptr || size == 0) to catch programming errors early.
| sb << "Continuing "; | ||
| } | ||
| desc += snprintf(desc, MAX_HEADER_LEN - (desc - buffer), "call on unexpected message for Call-Id '%s': ", id); | ||
| sb << "call on unexpected message for Call-Id '" << id << "': "; |
There was a problem hiding this comment.
Potential null pointer dereference if id is null. The operator<<(const char*) implementation checks for null, but silently ignoring a null id would result in an incomplete error message. Consider ensuring id is valid before building the message, or provide a default value like '' when id is null to make error messages more informative.
| sb << "call on unexpected message for Call-Id '" << id << "': "; | |
| sb << "call on unexpected message for Call-Id '" << (id ? id : "<unknown>") << "': "; |
wdoekes
left a comment
There was a problem hiding this comment.
Yea. I think this makes sense.
Best would be to remove all those static buffers, but that's far more invasive. So this could be a good start.
Should be faster and safer.
Fixes #815.