Fix buffer overflow in Windows process report #25348
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a buffer overflow vulnerability in the Windows process report implementation by replacing unsafe sprintf with snprintf. While the security improvement is valuable, the implementation introduces an API inconsistency and duplicates existing functionality.
Key changes:
- Added
bufferSizeparameter totoISOStringfunction in Windows implementation - Replaced
sprintfwithsnprintffor bounds checking - Updated call site to pass buffer size
Comments suppressed due to low confidence (1)
src/bun.js/bindings/BunProcessReportObjectWindows.cpp:53
- This function appears to be a duplicate implementation of the canonical
Bun::toISOStringalready defined inwtf-bindings.cpp(line 211). The canonical version is more robust as it:
- Uses
vm.dateCache.msToGregorianDateTime()instead ofgmtime() - Properly handles years outside the 0-9999 range per ES spec
- Returns the number of characters written
- Already uses
snprintfsafely
Consider removing this duplicate implementation and using the canonical version instead.
static void toISOString(JSC::VM& vm, double time, char* buffer, size_t bufferSize)
{
time_t seconds = static_cast<time_t>(time / 1000);
int milliseconds = static_cast<int>(time) % 1000;
struct tm* timeinfo = gmtime(&seconds);
snprintf(buffer, bufferSize, "%04d-%02d-%02dT%02d:%02d:%02d.%03dZ",
timeinfo->tm_year + 1900,
timeinfo->tm_mon + 1,
timeinfo->tm_mday,
timeinfo->tm_hour,
timeinfo->tm_min,
timeinfo->tm_sec,
milliseconds);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Helper function to convert time to ISO string | ||
| static void toISOString(JSC::VM& vm, double time, char* buffer) | ||
| static void toISOString(JSC::VM& vm, double time, char* buffer, size_t bufferSize) |
There was a problem hiding this comment.
The signature change to add bufferSize parameter creates an inconsistency with the canonical Bun::toISOString function declared in wtf-bindings.h (line 12) and implemented in wtf-bindings.cpp (line 211). The canonical version has the signature:
size_t toISOString(JSC::VM& vm, double date, char buffer[64]);This creates a name collision since both are in the Bun namespace. Consider either:
- Using the canonical
Bun::toISOStringfromwtf-bindings.cppwhich already usessnprintfsafely - Renaming this Windows-specific version to avoid the collision (e.g.,
toISOStringWindows)
The canonical version also handles edge cases better (years outside 0-9999 range).
| { | ||
| time_t seconds = static_cast<time_t>(time / 1000); | ||
| int milliseconds = static_cast<int>(time) % 1000; | ||
| struct tm* timeinfo = gmtime(&seconds); |
There was a problem hiding this comment.
Missing null check for gmtime() return value. The gmtime() function can return nullptr if the time value is invalid or out of range. Dereferencing a null pointer would cause a crash.
The canonical implementation in wtf-bindings.cpp avoids this by using vm.dateCache.msToGregorianDateTime() which is safer. Consider adding a null check:
struct tm* timeinfo = gmtime(&seconds);
if (!timeinfo) {
buffer[0] = '\0';
return;
}| struct tm* timeinfo = gmtime(&seconds); | |
| struct tm* timeinfo = gmtime(&seconds); | |
| if (!timeinfo) { | |
| buffer[0] = '\0'; | |
| return; | |
| } |
| // Helper function to convert time to ISO string | ||
| static void toISOString(JSC::VM& vm, double time, char* buffer) | ||
| static void toISOString(JSC::VM& vm, double time, char* buffer, size_t bufferSize) | ||
| { |
There was a problem hiding this comment.
The vm parameter is unused in this function. Consider adding UNUSED_PARAM(vm); at the beginning of the function to suppress compiler warnings, or better yet, use the canonical implementation from wtf-bindings.cpp which properly utilizes vm.dateCache for safer date/time conversion.
| { | |
| { | |
| UNUSED_PARAM(vm); |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoves a local ISO time-formatting helper from Changes
Pre-merge checks✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)src/**/*.{cpp,zig}📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
src/bun.js/bindings/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (22)📓 Common learnings📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-11-24T18:37:47.899ZApplied to files:
📚 Learning: 2025-12-02T05:59:51.485ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-12-02T05:59:51.485ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-10-01T21:59:54.571ZApplied to files:
📚 Learning: 2025-10-01T21:49:27.862ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-09-12T18:16:50.754ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-12-02T05:59:51.485ZApplied to files:
📚 Learning: 2025-11-24T18:35:25.883ZApplied to files:
📚 Learning: 2025-11-24T18:35:39.205ZApplied to files:
📚 Learning: 2025-12-02T05:59:51.485ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wmh
force-pushed
the
claude/fix-windows-process-report-buffer-overflow
branch
from
7a686c2 to
3e4e774
Compare
Remove duplicate toISOString helper and use the existing canonical version from wtf-bindings.h. The canonical version is more robust: - Already uses snprintf safely - Uses vm.dateCache.msToGregorianDateTime() instead of gmtime() - Handles edge cases like years outside 0-9999 range per ES spec - Returns character count for validation This addresses the CodeRabbit review feedback about API inconsistency and code duplication.
wmh
force-pushed
the
claude/fix-windows-process-report-buffer-overflow
branch
from
28e79d2 to
e098014
Compare
1 participant
Summary
This PR fixes a potential buffer overflow vulnerability in the Windows process report implementation by replacing unsafe
sprintfwithsnprintf.Changes
toISOStringhelper function to accept abufferSizeparametersprintfwithsnprintfto prevent buffer overflowsizeof(timeBuf)for proper bounds checkingSecurity Impact
The previous code used
sprintfwithout bounds checking when formatting timestamps, which could lead to buffer overflow if the formatted string exceeded the 64-byte buffer size. This change ensures the output is always truncated to fit within the allocated buffer.Testing
This is a Windows-only code path in
BunProcessReportObjectWindows.cpp. The change is minimal and maintains the same functionality while adding safety.