Skip to content

Conversation

assembler-0
Copy link
Owner

@assembler-0 assembler-0 commented Aug 14, 2025

Summary by CodeRabbit

  • New Features

    • Auto-detects and switches to VBE graphics mode, bypassing legacy VGA when available.
    • Adds printf-style formatted console and serial output with colored status badges.
    • Console color set expanded to a full 16-color palette with clearer defaults.
    • Shell gains a command to display a detailed kernel memory layout and stats.
    • Minor IPC/message handling improvements.
  • Chores

    • Added lightweight formatting module to the build.

Copy link

coderabbitai bot commented Aug 14, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Replaces the public va_list typedef with the compiler builtin, adds a printf-like formatter and integrates it into the kernel console (new variadic print APIs, badges, VBE-aware rendering), adds StringLength and IPC message helpers, exposes kernel linker symbols for a memory-layout command, and registers Format.c in the build.

Changes

Cohort / File(s) Summary
Stdarg builtin adoption
include/stdarg.h
Change typedef of va_list from typedef char* va_list; to typedef __builtin_va_list va_list;. va_start/va_arg remain builtins.
Console: VBE gating, colors, formatted APIs
kernel/etc/Console.c, kernel/etc/Console.h
Add use_vbe gating across cursor/screen ops and auto-detect VBE in ConsoleInit; introduce PrintKernelF, PrintKernelWarningF, PrintKernelErrorF, SerialWriteF, and PrintKernelBadge; replace limited color set with full 16-color VGA palette and remap default/special color macros.
Formatting subsystem
kernel/etc/Format.h, kernel/etc/Format.c
New lightweight printf-like formatter: FormatSpec struct, CHAR_BUFF, Format(const char*, va_list) plus helpers for parsing, padding, integers, strings, and characters; supports %d/%i, %u, %x/%X, %p, %s, %c, and %%.
String utilities
kernel/etc/StringOps.h, kernel/etc/StringOps.c
Add int StringLength(const char* str) and expose prototype.
Shell: memory layout command & symbols
kernel/etc/Shell.c
Add extern linker-symbol declarations for kernel sections and implement PrintKernelMemoryLayout(void); add layoutmem shell command entry and handling.
IPC updates
kernel/ipc/Ipc.h
Extend MessageQueue with tail and count; add static inline bool ToData(IpcMessage *data, const char *msg) which copies characters into message payload (no bounds checks).
Build integration
meson.build
Add kernel/etc/Format.c to kernel C sources so the formatter is compiled into the kernel.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Console
  participant Formatter
  participant VideoDriver
  participant Serial

  Caller->>Console: PrintKernelF(fmt, ...)
  Console->>Formatter: Format(fmt, va_list)
  Formatter-->>Console: formatted string
  Console->>Console: PrintKernelBadge(...) [optional]
  alt use_vbe == true
    Console->>VideoDriver: render via VBE path
  else
    Console->>VideoDriver: render via VGA text path
  end
  Console->>Serial: SerialWrite / SerialWriteF (mirror)
Loading
sequenceDiagram
  participant Boot
  participant Console
  participant VideoProbe

  Boot->>Console: ConsoleInit()
  Console->>VideoProbe: probe VBE support
  alt VBE present
    Console-->>Console: set use_vbe = true
  else
    Console-->>Console: set use_vbe = false (VGA)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Possibly related PRs

  • Development #83 — Same change set; touches identical files and symbols (stdarg, Format, Console formatted APIs).
  • THE vbe is cooked #62 — Overlaps console subsystem changes (color macros and console print functions).
  • Development #51 — Related to Shell.c modifications; both add/modify shell commands and execution handling.

Poem

I hop across the kernel’s stream,
New badges shining, colors gleam.
I craft my strings with %s and %d,
Send to VGA or VBE with glee.
A tiny rabbit, printing free 🐇✨

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 17ffb44 and fd69f19.

📒 Files selected for processing (4)
  • kernel/etc/Format.c (1 hunks)
  • kernel/etc/Shell.c (4 hunks)
  • kernel/etc/StringOps.c (1 hunks)
  • kernel/ipc/Ipc.h (3 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch Development

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🔭 Outside diff range comments (2)
include/stdarg.h (1)

8-11: Define va_copy to complete the stdarg surface

Many formatters need to duplicate va_list (e.g., probing width then formatting). Provide va_copy via the builtin to ensure portability and correctness.

Apply this diff:

 #define va_start(v,l)   __builtin_va_start(v,l)
 #define va_arg(v,t)     __builtin_va_arg(v,t)
 #define va_end(v)       __builtin_va_end(v)
+#define va_copy(d,s)    __builtin_va_copy(d,s)
kernel/etc/Console.c (1)

71-75: ClearScreen should respect current console color instead of hardcoded default

In VGA mode clearing uses VGA_COLOR_DEFAULT, ignoring the current color state. This regresses color consistency and contradicts previously reported issues around color washing.

Apply this diff:

-        const uint16_t blank = MakeVGAEntry(' ', VGA_COLOR_DEFAULT);
+        const uint16_t blank = MakeVGAEntry(' ', console.color);
🧹 Nitpick comments (5)
kernel/etc/StringOps.h (1)

9-9: Return type should be size_t for lengths; align with FastStrlen and avoid narrowing

String lengths are naturally size_t. Using int introduces unnecessary narrowing and inconsistency with FastStrlen.

Apply this diff:

-int StringLength(const char* str);
+size_t StringLength(const char* str);

Follow-up:

  • If you adopt this change, update the implementation in kernel/etc/StringOps.c and any call sites (e.g., Format.c) to handle size_t. I can help generate the patches across the repo.
kernel/etc/Console.h (2)

66-66: Typo: “formated” → “formatted”

Nit, but it’s a public header.

Apply this diff:

-// formated functions
+// formatted functions

67-70: Document lifetime/reentrancy expectations of formatted print wrappers

These take varargs and (per Console.c snippets) call Format() returning a char*. If Format() uses a static buffer, these functions are not reentrant or thread-safe and can be clobbered by nested/parallel calls (e.g., interrupts, multiple CPUs).

Recommendations:

  • Add a brief note in this header stating whether the formatted print functions are reentrant and the lifetime of the returned buffer from Format().
  • If non-reentrant by design, consider guarding use in interrupt context or switching Format() to write into a caller-provided buffer or the console driver’s internal buffer to avoid shared state issues.
    Would you like me to draft the header docs and a reentrant variant signature?
kernel/etc/Console.c (1)

155-164: Remove no-op save/restore of console.color

original_color is captured and later restored, but no color changes occur in this function. This is dead code and adds noise.

Apply this diff:

     } else {
-        const uint8_t original_color = console.color;
         for (const char* p = str; *p; p++) {
             ConsolePutchar(*p);
         }
-        console.color = original_color;
     }
kernel/etc/Format.c (1)

56-82: PadString is O(n^2) and unbounded; consider a bounded, linear-time version

Repeatedly calling StringLength and concatenating one char at a time is inefficient and assumes the destination is large enough. Since we’re moving to a bounded top-level formatter, PadString should also be made capacity-aware and write in linear time.

I can propose a bounded PadString(dest, dest_cap, ...) variant that avoids repeated scans and integrates with the Append helpers used in VFormat.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1dd44ae and 17ffb44.

📒 Files selected for processing (8)
  • include/stdarg.h (1 hunks)
  • kernel/etc/Console.c (9 hunks)
  • kernel/etc/Console.h (2 hunks)
  • kernel/etc/Format.c (1 hunks)
  • kernel/etc/Format.h (1 hunks)
  • kernel/etc/StringOps.c (1 hunks)
  • kernel/etc/StringOps.h (1 hunks)
  • meson.build (1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-10T06:03:22.048Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.048Z
Learning: In VoidFrame kernel's VBEConsoleClear() function, the color is hardcoded to 0x08 (gray on black) instead of using the current console color state, causing color washing issues. The function should use console.color or the current color state to maintain color consistency across screen clears.

Applied to files:

  • kernel/etc/Console.h
  • kernel/etc/Console.c
📚 Learning: 2025-08-10T06:31:43.762Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:31:43.762Z
Learning: In VoidFrame kernel's VBEConsolePutChar() function, the color_buffer must be updated with the current attribute byte whenever a character is stored in the buffer. Without this, characters inherit stale colors from VBEConsoleClear() causing all text to appear in the wrong color (gray/0x08) after refresh operations.

Applied to files:

  • kernel/etc/Console.h
  • kernel/etc/Console.c
📚 Learning: 2025-08-10T06:03:22.048Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.048Z
Learning: In VoidFrame kernel, the console color state (current_color) needs to be explicitly preserved and restored when switching between display modes or clearing the screen. ConsoleSetColor() should update both text mode and VBE mode color states when vbe_enabled is true.

Applied to files:

  • kernel/etc/Console.h
  • kernel/etc/Console.c
📚 Learning: 2025-08-10T06:03:22.048Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.048Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.

Applied to files:

  • kernel/etc/Console.c
📚 Learning: 2025-08-10T06:03:22.048Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.048Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.

Applied to files:

  • kernel/etc/Console.c
🧬 Code Graph Analysis (5)
kernel/etc/StringOps.h (1)
kernel/etc/StringOps.c (1)
  • StringLength (3-7)
kernel/etc/Format.h (1)
kernel/etc/Format.c (1)
  • Format (177-263)
kernel/etc/Console.h (1)
kernel/etc/Console.c (4)
  • PrintKernelF (228-235)
  • SerialWriteF (255-262)
  • PrintKernelErrorF (246-253)
  • PrintKernelWarningF (237-244)
kernel/etc/Console.c (2)
kernel/etc/Format.c (1)
  • Format (177-263)
drivers/Serial.c (1)
  • SerialWrite (129-138)
kernel/etc/Format.c (2)
kernel/etc/StringOps.c (1)
  • StringLength (3-7)
kernel/memory/MemOps.c (4)
  • strcpy (5-7)
  • strcat (9-12)
  • htoa (14-22)
  • itoa (24-39)
🔇 Additional comments (5)
kernel/etc/Console.h (2)

9-24: 16-color VGA palette addition looks correct

The constants map to the standard VGA 0x0–0xF palette. This expands flexibility without changing existing attribute packing.


26-29: Good aliasing of semantic colors to palette

DEFAULT/SUCCESS/ERROR/WARNING aliases make usage clearer while keeping palette values centralized.

include/stdarg.h (1)

6-6: Correct: use compiler builtin va_list for freestanding envs

Switching to __builtin_va_list avoids ABI pitfalls inherent in typedef’ing va_list as a raw pointer.

meson.build (1)

83-83: Build integration for Format.c is correct

Including kernel/etc/Format.c in c_sources ensures the new formatting subsystem is linked. No further build adjustments needed.

kernel/etc/Console.c (1)

143-149: Verify VBE color handling paths align with known pitfalls

Given prior issues:

  • VBEConsoleClear must use the current color, not a hardcoded attribute.
  • VBEConsolePutChar must update any color/attribute buffers, or characters inherit stale colors.

Please verify VBEConsoleClear() and VBEConsolePutChar() implement the above; otherwise badge and warning/error colors will display incorrectly in VBE mode.

Would you like me to scan VBEConsole.c to confirm color propagation and propose a patch if needed?

Also applies to: 63-68, 103-107

Comment on lines +228 to +262
void PrintKernelF(const char* format, ...) {
va_list args;
va_start(args, format);
char* formatted = Format(format, args);
va_end(args);

PrintKernel(formatted);
}

void PrintKernelWarningF(const char* format, ...) {
va_list args;
va_start(args, format);
char* formatted = Format(format, args);
va_end(args);

PrintKernelWarning(formatted);
}

void PrintKernelErrorF(const char* format, ...) {
va_list args;
va_start(args, format);
char* formatted = Format(format, args);
va_end(args);

PrintKernelError(formatted);
}

void SerialWriteF(const char* format, ...) {
va_list args;
va_start(args, format);
char* formatted = Format(format, args);
va_end(args);

SerialWrite(formatted);
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Use-after-return: Format() result is invalid; format into a local buffer and pass it on

All four variadic wrappers pass the pointer returned by Format() to printers. With the current implementation, that pointer refers to a stack buffer that no longer exists, leading to undefined behavior, random output, and potential faults under optimization.

Switch to caller-allocated buffers and use a bounded va_list-based formatter.

Apply this diff (assumes the refactored VFormat API in Format.h/Format.c):

 void PrintKernelF(const char* format, ...) {
-    va_list args;
-    va_start(args, format);
-    char* formatted = Format(format, args);
-    va_end(args);
-
-    PrintKernel(formatted);
+    char buf[CHAR_BUFF];
+    va_list args;
+    va_start(args, format);
+    VFormat(buf, (int)sizeof(buf), format, args);
+    va_end(args);
+    PrintKernel(buf);
 }
 
 void PrintKernelWarningF(const char* format, ...) {
-    va_list args;
-    va_start(args, format);
-    char* formatted = Format(format, args);
-    va_end(args);
-
-    PrintKernelWarning(formatted);
+    char buf[CHAR_BUFF];
+    va_list args;
+    va_start(args, format);
+    VFormat(buf, (int)sizeof(buf), format, args);
+    va_end(args);
+    PrintKernelWarning(buf);
 }
 
 void PrintKernelErrorF(const char* format, ...) {
-    va_list args;
-    va_start(args, format);
-    char* formatted = Format(format, args);
-    va_end(args);
-
-    PrintKernelError(formatted);
+    char buf[CHAR_BUFF];
+    va_list args;
+    va_start(args, format);
+    VFormat(buf, (int)sizeof(buf), format, args);
+    va_end(args);
+    PrintKernelError(buf);
 }
 
 void SerialWriteF(const char* format, ...) {
-    va_list args;
-    va_start(args, format);
-    char* formatted = Format(format, args);
-    va_end(args);
-
-    SerialWrite(formatted);
+    char buf[CHAR_BUFF];
+    va_list args;
+    va_start(args, format);
+    VFormat(buf, (int)sizeof(buf), format, args);
+    va_end(args);
+    SerialWrite(buf);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void PrintKernelF(const char* format, ...) {
va_list args;
va_start(args, format);
char* formatted = Format(format, args);
va_end(args);
PrintKernel(formatted);
}
void PrintKernelWarningF(const char* format, ...) {
va_list args;
va_start(args, format);
char* formatted = Format(format, args);
va_end(args);
PrintKernelWarning(formatted);
}
void PrintKernelErrorF(const char* format, ...) {
va_list args;
va_start(args, format);
char* formatted = Format(format, args);
va_end(args);
PrintKernelError(formatted);
}
void SerialWriteF(const char* format, ...) {
va_list args;
va_start(args, format);
char* formatted = Format(format, args);
va_end(args);
SerialWrite(formatted);
}
void PrintKernelF(const char* format, ...) {
char buf[CHAR_BUFF];
va_list args;
va_start(args, format);
VFormat(buf, (int)sizeof(buf), format, args);
va_end(args);
PrintKernel(buf);
}
void PrintKernelWarningF(const char* format, ...) {
char buf[CHAR_BUFF];
va_list args;
va_start(args, format);
VFormat(buf, (int)sizeof(buf), format, args);
va_end(args);
PrintKernelWarning(buf);
}
void PrintKernelErrorF(const char* format, ...) {
char buf[CHAR_BUFF];
va_list args;
va_start(args, format);
VFormat(buf, (int)sizeof(buf), format, args);
va_end(args);
PrintKernelError(buf);
}
void SerialWriteF(const char* format, ...) {
char buf[CHAR_BUFF];
va_list args;
va_start(args, format);
VFormat(buf, (int)sizeof(buf), format, args);
va_end(args);
SerialWrite(buf);
}
🤖 Prompt for AI Agents
In kernel/etc/Console.c around lines 228-262, the wrappers call Format() which
returns a pointer into a stack buffer that is no longer valid; replace that
pattern by allocating a caller-owned buffer on the stack (e.g. char
buf[BUFFER_SIZE]), start the va_list, call the refactored VFormat(buffer, sizeof
buffer, format, args) to write into the bounded buffer, end the va_list, then
pass the buffer to PrintKernel/PrintKernelWarning/PrintKernelError/SerialWrite;
ensure you use va_start and va_end correctly and choose a safe buffer size to
avoid overflow.

Comment on lines +138 to +144
// Apply width and padding
if (spec->width > 0) {
PadString(buffer, final_buffer, spec->width, spec->pad_char, spec->left_align);
} else {
strcpy(buffer, final_buffer);
}
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Zero-padding semantics for signed/prefixed numbers are incorrect

Applying left padding with pad_char '0' to the whole final string yields "000-123" instead of "-000123". The '0' flag pads after sign/prefix and before digits.

Suggestion (sketch): when zero-padding is active, width > len, !left_align, compute pad_len and construct: [sign][prefix][zeros][digits]. Only use space-padding with PadString. I can provide a concrete patch if you confirm desired behavior aligns with printf.

🤖 Prompt for AI Agents
In kernel/etc/Format.c around lines 138 to 144, the current branch applies
zero-padding to the whole formatted string producing "000-123" instead of
"-000123"; change the logic so when spec->pad_char == '0', spec->width >
current_length and !spec->left_align you must detect and separate any sign
('+'/'-'/' ') and any prefix (like "0x"/"0X") from the digit portion, compute
pad_len = spec->width - current_length, then build final_buffer as
[sign][prefix][pad_len zeros][digits]; for all other cases (space padding or
left align) continue to use PadString or strcpy as before.

Comment on lines +152 to +155
// Apply precision (max characters)
if (spec->precision >= 0) {
temp_buffer[spec->precision] = '\0';
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

String precision can overflow the temp buffer; clamp before truncating

If precision >= sizeof(temp_buffer), temp_buffer[precision] writes out of bounds. Clamp precision to 511.

Apply this diff:

-    // Apply precision (max characters)
-    if (spec->precision >= 0) {
-        temp_buffer[spec->precision] = '\0';
-    }
+    // Apply precision (max characters)
+    if (spec->precision >= 0) {
+        int max = (int)sizeof(temp_buffer) - 1;
+        int cut = spec->precision < max ? spec->precision : max;
+        temp_buffer[cut] = '\0';
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Apply precision (max characters)
if (spec->precision >= 0) {
temp_buffer[spec->precision] = '\0';
}
// Apply precision (max characters)
if (spec->precision >= 0) {
int max = (int)sizeof(temp_buffer) - 1;
int cut = spec->precision < max ? spec->precision : max;
temp_buffer[cut] = '\0';
}
🤖 Prompt for AI Agents
In kernel/etc/Format.c around lines 152 to 155, the code truncates temp_buffer
using spec->precision without ensuring precision is within buffer bounds; when
spec->precision >= sizeof(temp_buffer) this writes out of bounds. Before setting
temp_buffer[spec->precision] = '\0', clamp spec->precision to at most
sizeof(temp_buffer) - 1 (e.g. if (spec->precision >= (int)sizeof(temp_buffer))
spec->precision = sizeof(temp_buffer) - 1;) so the index is always valid, then
proceed to null-terminate.

Comment on lines +177 to +183
char* Format(const char* format, va_list args) {
char output_buffer[CHAR_BUFF];
output_buffer[0] = '\0';

const char* f = format;
char temp_buffer[512];

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Critical: returning pointer to a stack buffer and unbounded strcat leads to UB and overflow

Format() builds into a local stack array and returns its address. All callers dereference a dangling pointer. Additionally, strcat/strcpy are used without capacity checks, risking buffer overflows into adjacent stack frames.

Refactor to:

  • Write into a caller-provided buffer (bounded).
  • Replace strcat/strcpy uses in the top-level formatter with bounded appends.

Apply this diff to introduce bounded append helpers and replace Format with VFormat:

 #include "stdint.h"
 
-
+// Bounded append helpers
+static inline void AppendStr(char* out, int out_cap, const char* src, int* out_len) {
+    if (out_cap <= 0) return;
+    int i = 0;
+    while (src[i] != '\0' && *out_len < out_cap - 1) {
+        out[(*out_len)++] = src[i++];
+    }
+    out[*out_len] = '\0';
+}
+
+static inline void AppendChar(char* out, int out_cap, char c, int* out_len) {
+    if (out_cap <= 0) return;
+    if (*out_len < out_cap - 1) {
+        out[(*out_len)++] = c;
+        out[*out_len] = '\0';
+    }
+}
@@
-char* Format(const char* format, va_list args) {
-    char output_buffer[CHAR_BUFF];
-    output_buffer[0] = '\0';
-
-    const char* f = format;
-    char temp_buffer[512];
+int VFormat(char* out, int out_size, const char* format, va_list args) {
+    if (!out || out_size <= 0) return 0;
+    out[0] = '\0';
+    int out_len = 0;
+
+    const char* f = format;
+    char temp_buffer[512];
@@
-            if (*f == '%') {
+            if (*f == '%') {
                 // Literal '%'
-                strcat(output_buffer, "%");
+                AppendChar(out, out_size, '%', &out_len);
                 f++;
                 continue;
             }
@@
-                    FormatInteger(temp_buffer, value, &spec, 10, 0);
-                    strcat(output_buffer, temp_buffer);
+                    FormatInteger(temp_buffer, value, &spec, 10, 0);
+                    AppendStr(out, out_size, temp_buffer, &out_len);
                     break;
                 }
@@
-                    FormatInteger(temp_buffer, value, &spec, 10, 1);
-                    strcat(output_buffer, temp_buffer);
+                    FormatInteger(temp_buffer, value, &spec, 10, 1);
+                    AppendStr(out, out_size, temp_buffer, &out_len);
                     break;
                 }
@@
-                    FormatInteger(temp_buffer, value, &spec, 16, 1);
-                    strcat(output_buffer, temp_buffer);
+                    FormatInteger(temp_buffer, value, &spec, 16, 1);
+                    AppendStr(out, out_size, temp_buffer, &out_len);
                     break;
                 }
@@
-                    spec.show_prefix = 1;  // Always show 0x for pointers
-                    FormatInteger(temp_buffer, (uintptr_t)ptr, &spec, 16, 1);
-                    strcat(output_buffer, temp_buffer);
+                    spec.show_prefix = 1;  // Always show 0x for pointers
+                    FormatInteger(temp_buffer, (uintptr_t)ptr, &spec, 16, 1);
+                    AppendStr(out, out_size, temp_buffer, &out_len);
                     break;
                 }
@@
-                    FormatString(temp_buffer, str, &spec);
-                    strcat(output_buffer, temp_buffer);
+                    FormatString(temp_buffer, str, &spec);
+                    AppendStr(out, out_size, temp_buffer, &out_len);
                     break;
                 }
@@
-                    FormatCharacter(temp_buffer, (char)c, &spec);
-                    strcat(output_buffer, temp_buffer);
+                    FormatCharacter(temp_buffer, (char)c, &spec);
+                    AppendStr(out, out_size, temp_buffer, &out_len);
                     break;
                 }
@@
-                    temp_buffer[0] = *f;
-                    temp_buffer[1] = '\0';
-                    strcat(output_buffer, temp_buffer);
+                    AppendChar(out, out_size, *f, &out_len);
                     break;
             }
 
             f++;
         } else {
             // Regular character
-            int len = StringLength(output_buffer);
-            output_buffer[len] = *f;
-            output_buffer[len + 1] = '\0';
+            AppendChar(out, out_size, *f, &out_len);
             f++;
         }
     }
     
-    return output_buffer;
+    return out_len;
 }

Also applies to: 260-262


#define CHAR_BUFF 8192

char* Format(const char* format, va_list args) ;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

API returns a dangling pointer; redefine to write into caller-provided buffer

The declared API invites implementations that return pointers with unclear lifetime. The current implementation in Format.c returns a pointer to a stack buffer, causing immediate use-after-return UB in all callers.

Redesign the API to write into a caller-provided buffer with an explicit capacity and return the number of bytes written. This also enables safe, bounded writes and avoids global/static buffers.

Apply this diff:

-#define CHAR_BUFF 8192
-
-char* Format(const char* format, va_list args) ;
+#define CHAR_BUFF 8192
+
+// Writes formatted output into 'out' (up to out_size-1 chars + NUL).
+// Returns number of characters written (excluding NUL), truncated if >= out_size.
+int VFormat(char* out, int out_size, const char* format, va_list args);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
char* Format(const char* format, va_list args) ;
#define CHAR_BUFF 8192
// Writes formatted output into 'out' (up to out_size-1 chars + NUL).
// Returns number of characters written (excluding NUL), truncated if >= out_size.
int VFormat(char* out, int out_size, const char* format, va_list args);
🤖 Prompt for AI Agents
In kernel/etc/Format.h around line 18, the current declaration "char*
Format(const char* format, va_list args);" returns a pointer with unclear
lifetime (and your implementation returns a stack buffer) — change the API to
write into a caller-provided buffer: replace the prototype with something like
"size_t Format(char* dst, size_t dstSize, const char* format, va_list args);"
implement the function using vsnprintf to safely write at most dstSize bytes and
ensure null-termination, return the number of bytes that would have been written
(or the number actually written) as size_t, and update all callers to
allocate/provide buffers and handle the returned length accordingly to avoid
dangling-pointer UB.

@assembler-0 assembler-0 merged commit 330eb1b into main Aug 14, 2025
1 check was pending
This was referenced Sep 1, 2025
Merged
@coderabbitai coderabbitai bot mentioned this pull request Sep 9, 2025
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.

1 participant