THE vbe is cooked #62
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change refactors panic and interrupt exception handling across the kernel. It centralizes panic reporting into a modular, console-based system, introduces formatted panic messages for CPU exceptions, adds string manipulation utilities, and updates the VESA BIOS extension to support a panic splash image. Several header and implementation files are updated for API consistency and code clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant CPU_Exception
participant InterruptHandler
participant PanicFromInterrupt
participant KernelPanicHandler
participant Console/Serial
CPU_Exception->>InterruptHandler: Exception occurs (e.g., #GP, #PF)
InterruptHandler->>PanicFromInterrupt: Build formatted panic message, pass Registers
PanicFromInterrupt->>KernelPanicHandler: Pass message and CPU context
KernelPanicHandler->>Console/Serial: Print structured panic info, system stats
KernelPanicHandler->>VESA BIOS (optional): Show panic splash
KernelPanicHandler->>KernelPanicHandler: Halt system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (15)
kernel/memory/MemOps.h (1)
11-14: Remove duplicatedstrcatimplementation
kernel/core/Panic.cstill contains its own staticstrcat; now that a shared version exists, keep only one to avoid divergence.kernel/memory/MemOps.c (3)
1-4: Prune unused includes
Cpu.handPanic.hare not referenced in this file; dropping them trims compile time and dependency surface.
14-22: Document required buffer size forhtoaThe function unconditionally writes 19 bytes (incl. NUL). Callers must guarantee
buffer >= 19, but that isn’t stated anywhere. Add a comment or switch to a#define HEX_BUF_LEN 19exposed in the header.
24-39: Permit signed values or clarify API
itoaonly handles unsigned numbers. Either rename touitoa/ document that behaviour, or extend to signed 64-bit to avoid surprises.meson.build (1)
111-113: Automate object list & verify file presenceHard-coding every splash*/panic object scales poorly and risks missing files. Generate the list programmatically:
obj_sources += files(glob(src_root + '/kernel/etc/splash*.o')) obj_sources += files(src_root + '/kernel/etc/panic.o')Also ensure
kernel/etc/panic.ois committed; build will fail silently otherwise.kernel/etc/Console.h (1)
8-14:VGA_COLOR_SUCCESSis now orphaned
PrintKernelSuccessswitched toVGA_COLOR_WHITE, leavingVGA_COLOR_SUCCESSapparently unused.
If the green-ish success colour is no longer desired, please drop the macro; otherwise updatePrintKernelSuccessto keep semantic colouring consistent.drivers/VesaBIOSExtension.c (1)
349-359:VBEShowPanicdraws pixel-by-pixel in a tight loopCalling
VBEPutPixelper pixel incurs two levels of function overhead and colour-mapping for every write, making the splash extremely slow on higher resolutions.Consider memcpy’ing a pre-converted framebuffer line or doing a row-wise loop that writes directly into
vbe_info.framebufferonce the image has been colour-mapped.kernel/etc/Console.c (3)
165-172:SetVBEColorsnow unusedWith
ConsoleSetColormade public,SetVBEColorsis never invoked, so thevbe_fg_color/vbe_bg_colorcache is stale.
Either callSetVBEColors(color)inside the VBE branch or delete the dead code.
192-196: Success messages no longer greenSwitching to
VGA_COLOR_WHITEremoves the visual cue that a message represents success.
If this was intentional, deleteVGA_COLOR_SUCCESS; if not, revert to keep the green highlight.
264-265: Serial output adds unconditional newline
SerialWrite(str); SerialWrite("\n");means callers can’t control newlines and console/serial logs may diverge.
Recommend letting the caller append\nas needed to mirrorPrintKernelbehaviour.kernel/core/Panic.h (2)
9-11: Guard likely/unlikely macros to avoid redefinition warningsWrap with ifndef to prevent clashes when headers are included in different orders.
-#define likely(x) __builtin_expect(!!(x), 1) -#define unlikely(x) __builtin_expect(!!(x), 0) +#ifndef likely +#define likely(x) __builtin_expect(!!(x), 1) +#endif +#ifndef unlikely +#define unlikely(x) __builtin_expect(!!(x), 0) +#endif
30-33: Strengthen PanicFromInterrupt contract and keep types consistent
- Consider marking parameters non-null to catch misuse at compile time.
- Ensure the definition uses the same spelled type (struct Registers) to avoid confusion if the typedef is absent.
-void __attribute__((noreturn)) PanicFromInterrupt(const char* message, struct Registers* regs); +void __attribute__((noreturn, nonnull(1,2))) PanicFromInterrupt(const char* message, struct Registers* regs);If nonnull isn’t desirable globally, at least keep the definition spelling consistent.
kernel/core/Panic.c (3)
102-103: Remove unused variablecol2 is defined but never used.
- const int col1 = margin; - const int col2 = margin + 28; + const int col1 = margin;
143-144: Replace backspace print with a real spacer or drop itPrinting "\b\b" is odd for a UI spacer and may have undefined effects in your console. Either print an empty line or remove it.
- PrintKernelAt("\b\b", line++, col1); + PrintKernelAt("", line++, col1);
146-159: Ensure diagnostic dumps are panic-safe (no locks/allocations/IRQs needed)DumpSchedulerState, DumpPerformanceStats, GetDetailedMemoryStats/PrintVMemStats/PrintHeapStats must not take locks, allocate, or wait on interrupts. With CLI set, any IRQ-dependent path will deadlock.
Please verify these functions are:
- lock-free (or use try-lock with timeouts),
- allocation-free,
- IRQ-independent.
If not, reduce to a minimal, known-safe subset (e.g., serial-only prints of pre-collected counters).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kernel/etc/panic.ois excluded by!**/*.o
📒 Files selected for processing (12)
arch/x86_64/interrupts/Interrupts.c(2 hunks)drivers/VesaBIOSExtension.c(4 hunks)drivers/VesaBIOSExtension.h(1 hunks)kernel/core/Kernel.c(1 hunks)kernel/core/Panic.c(1 hunks)kernel/core/Panic.h(2 hunks)kernel/etc/Console.c(4 hunks)kernel/etc/Console.h(2 hunks)kernel/etc/VBEConsole.h(1 hunks)kernel/memory/MemOps.c(1 hunks)kernel/memory/MemOps.h(1 hunks)meson.build(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.917Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
📚 Learning: 2025-08-09T04:47:56.871Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#53
File: kernel/core/Kernel.c:256-259
Timestamp: 2025-08-09T04:47:56.871Z
Learning: The VoidFrame kernel memory crash occurs when VIRT_ADDR_SPACE_START is set to 0xFFFF800000000000ULL instead of 0xFFFFFFFF80000000ULL, causing the heap to allocate 128TB away from where the kernel is mapped, resulting in page faults when accessing heap memory.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-09T10:31:44.413Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#58
File: drivers/VesaBIOSExtension.c:77-85
Timestamp: 2025-08-09T10:31:44.413Z
Learning: In VoidFrame kernel, process stacks are allocated using VMemAllocWithGuards() which returns virtual addresses, but TerminateProcess incorrectly uses FreePage() (which expects physical addresses) to free them, causing "Page index out of bounds" errors. The correct function to use is VMemFreeWithGuards(proc->stack, STACK_SIZE).
Applied to files:
kernel/core/Kernel.carch/x86_64/interrupts/Interrupts.ckernel/core/Panic.c
📚 Learning: 2025-07-16T01:04:23.790Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#47
File: kernel/core/Kernel.c:434-434
Timestamp: 2025-07-16T01:04:23.790Z
Learning: VMemAlloc() in VoidFrame kernel fails with error code -2 (VMEM_ERROR_INVALID_ADDR) due to virtual address space layout conflict where the heap start address (_bss_end + KERNEL_VIRTUAL_OFFSET + 1MB margin) exceeds VIRT_ADDR_SPACE_END (0xFFFFFFFF80000000ULL), causing VMemMap validation to fail.
Applied to files:
kernel/core/Kernel.ckernel/core/Panic.c
📚 Learning: 2025-08-10T02:19:46.917Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.917Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/etc/Console.harch/x86_64/interrupts/Interrupts.ckernel/core/Panic.c
📚 Learning: 2025-07-08T00:36:50.297Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.297Z
Learning: In IdtSetGate function calls, the parameters are: (interrupt_vector, handler_address, selector, flags). The interrupt vector (like 0x80) is the first argument, and the selector (like 0x08 for kernel code segment) is the third argument.
Applied to files:
arch/x86_64/interrupts/Interrupts.c
📚 Learning: 2025-08-09T09:29:00.591Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#57
File: kernel/memory/VMem.h:64-66
Timestamp: 2025-08-09T09:29:00.591Z
Learning: In the VoidFrame kernel, removing locks from KernelMemoryAlloc/KernelMemoryFree causes heap corruption where shell command strings (like "clear" = 0x61656C63) overwrite heap block magic values due to race conditions between allocation and free operations.
Applied to files:
kernel/core/Panic.c
🧬 Code Graph Analysis (6)
kernel/etc/Console.h (1)
kernel/etc/Console.c (1)
ConsoleSetColor(166-172)
drivers/VesaBIOSExtension.h (1)
drivers/VesaBIOSExtension.c (2)
VBEShowPanic(349-359)delay(69-73)
kernel/memory/MemOps.h (2)
kernel/memory/MemOps.c (4)
strcpy(5-7)strcat(9-12)htoa(14-22)itoa(24-39)kernel/core/Panic.c (1)
strcat(62-65)
kernel/etc/Console.c (1)
drivers/Serial.c (1)
SerialWrite(129-138)
arch/x86_64/interrupts/Interrupts.c (2)
kernel/memory/MemOps.c (4)
htoa(14-22)strcpy(5-7)strcat(9-12)itoa(24-39)kernel/core/Panic.c (1)
strcat(62-65)
kernel/memory/MemOps.c (1)
kernel/core/Panic.c (1)
strcat(62-65)
🔇 Additional comments (6)
kernel/core/Kernel.c (1)
504-504: Cosmetic indent fix looks goodNo functional impact; improves readability.
kernel/etc/Console.h (1)
45-45: Header/impl consistency looks good
ConsoleSetColoris now public and the prototype is correctly added here. No further issues.kernel/etc/VBEConsole.h (1)
6-12: Addition looks fineThe new
VBE_CONSOLE_DEFAULT_COLORmacro is self-explanatory and does not clash with existing names.arch/x86_64/interrupts/Interrupts.c (1)
54-107: Exception message refactor looks solidCentralised panic formatting reduces code size and stack use; buffer lengths are adequate.
kernel/core/Panic.c (2)
164-172: LGTM on final halt/serial emitSerial emit before hlt loop is good; loop with hlt after cli is correct to quiesce the CPU.
3-13: No missing prototypes or include issues found
delay()is declared in drivers/VesaBIOSExtension.h, which Panic.c already includes.DumpSchedulerState()andDumpPerformanceStats()are declared in kernel/process/Process.h, which Panic.c includes.struct Registersis forward-declared in Panic.h and brought in via Cpu.h, so including Process.h for the dump functions is appropriate.No changes required.
| void VBEShowPanic(void); | ||
| void delay(uint32_t ms); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify delay units & namespace the helper
delay(uint32_t ms) implies time in ms yet the implementation takes an iteration count. Either rename to VBEBusyWait(uint32_t loops) or scale the loop to approximate milliseconds and document the approximation.
This avoids misuse elsewhere in the kernel.
🤖 Prompt for AI Agents
In drivers/VesaBIOSExtension.h at lines 45-46, the function delay(uint32_t ms)
is misleading because it suggests a delay in milliseconds, but the
implementation uses an iteration count. Rename the function to
VBEBusyWait(uint32_t loops) to reflect its actual behavior or modify the
implementation to scale the loops to approximate milliseconds and add
documentation explaining the approximation. This change will prevent misuse and
clarify the function's purpose.
| // --- Self-Contained String Formatting Helpers --- | ||
| // These are kept here to format strings before printing, as the Console API prints directly. | ||
|
|
||
| for (int i = 0; i < SCREEN_WIDTH * SCREEN_HEIGHT; i++) { | ||
| video_memory[i] = red_attr | ' '; | ||
| static void U64ToHexStr(uint64_t value, char* buffer) { | ||
| const char* hex_chars = "0123456789ABCDEF"; | ||
| buffer[0] = '0'; | ||
| buffer[1] = 'x'; | ||
| for (int i = 0; i < 16; i++) { | ||
| buffer[2 + i] = hex_chars[(value >> (60 - i * 4)) & 0xF]; | ||
| } | ||
| buffer[18] = '\0'; | ||
| } | ||
|
|
||
| void PanicPrint(int x, int y, const char* str, uint8_t color) { | ||
| uint16_t* video_memory = (uint16_t*)(VIDEO_MEMORY + (y * SCREEN_WIDTH + x) * 2); | ||
| uint16_t attr = (color << 8); | ||
|
|
||
| while (*str) { | ||
| *video_memory++ = attr | *str++; | ||
| } | ||
| } | ||
|
|
||
| void PanicPrintCentered(int y, const char* str, uint8_t color) { | ||
| int len = 0; | ||
| while (str[len] != '\0') len++; | ||
| int x = (SCREEN_WIDTH - len) / 2; | ||
| if (x < 0) x = 0; | ||
| PanicPrint(x, y, str, color); | ||
| } | ||
|
|
||
| // NEW: Helper to print decimal numbers (for line numbers) | ||
| void PanicPrintDec(int x, int y, uint32_t value, uint8_t color) { | ||
| char buffer[11]; // Max 10 digits for a 32-bit unsigned int + null terminator | ||
| char* p = buffer + sizeof(buffer) - 1; | ||
| *p = '\0'; | ||
|
|
||
| static void U32ToDecStr(uint32_t value, char* buffer) { | ||
| if (value == 0) { | ||
| *--p = '0'; | ||
| } else { | ||
| while (value > 0) { | ||
| *--p = '0' + (value % 10); | ||
| value /= 10; | ||
| } | ||
| buffer[0] = '0'; | ||
| buffer[1] = '\0'; | ||
| return; | ||
| } | ||
| PanicPrint(x, y, p, color); | ||
| } | ||
|
|
||
| void PanicPrintHex(int x, int y, uint64_t value, uint8_t color) { | ||
| const char hex[] = "0123456789ABCDEF"; | ||
| char buffer[19] = "0x"; | ||
|
|
||
| // Position the pointer to the end of the buffer part for hex digits | ||
| char* p = buffer + 2; | ||
| char int_buffer[11]; | ||
| char* p = &int_buffer[10]; | ||
| *p = '\0'; | ||
|
|
||
| for (int i = 15; i >= 0; i--) { | ||
| p[15-i] = hex[(value >> (i * 4)) & 0xF]; | ||
| } | ||
| p[16] = '\0'; | ||
| uint32_t temp = value; | ||
| do { | ||
| *--p = '0' + (temp % 10); | ||
| temp /= 10; | ||
| } while (temp > 0); | ||
|
|
||
| PanicPrint(x, y, buffer, color); | ||
| // Basic strcpy | ||
| char* d = buffer; | ||
| while ((*d++ = *p++)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicating string helpers; reuse shared MemOps and reduce code surface
These helpers duplicate functionality commonly provided by shared utilities (per PR summary). Prefer central helpers to remove duplication and reduce bugs.
- Replace U64ToHexStr/U32ToDecStr with htoa/itoa (or equivalent) from MemOps.h.
- If MemOps isn’t available here, at least consolidate these helpers in one reusable module.
I can generate a patch switching to MemOps.h if you confirm the exact function signatures.
🤖 Prompt for AI Agents
In kernel/core/Panic.c from lines 26 to 59, the U64ToHexStr and U32ToDecStr
functions duplicate string formatting helpers already available in MemOps.h as
htoa and itoa. To fix this, remove these local helper functions and replace
their usage with the corresponding MemOps.h functions, ensuring you include
MemOps.h and match the function signatures. If MemOps.h is not currently
included, add the include directive. This consolidation reduces code duplication
and potential bugs.
kernel/core/Panic.c
Outdated
| // Simple helper to concatenate strings. | ||
| static void strcat(char* dest, const char* src) { | ||
| while (*dest) dest++; | ||
| while ((*dest++ = *src++)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Unsafe strcat risks stack buffer overflow in panic path
temp_buffer is 128 bytes, but concatenations include unbounded message and file paths. Replace with a bounds-checked appender.
Apply:
-// Simple helper to concatenate strings.
-static void strcat(char* dest, const char* src) {
- while (*dest) dest++;
- while ((*dest++ = *src++));
-}
+// Bounded append: appends src to dest ensuring NUL-termination.
+static void bufcat(char* dest, size_t cap, const char* src) {
+ size_t i = 0;
+ while (i < cap && dest[i]) i++;
+ if (i == cap) return; // dest not NUL-terminated; bail.
+ size_t j = 0;
+ while (src[j] && i + 1 < cap) dest[i++] = src[j++];
+ dest[i] = '\0';
+}Example usage (apply pattern to all call sites):
temp_buffer[0] = '\0';
bufcat(temp_buffer, sizeof temp_buffer, "Reason: ");
if (message) bufcat(temp_buffer, sizeof temp_buffer, message);🤖 Prompt for AI Agents
In kernel/core/Panic.c around lines 61 to 66, the current strcat function is
unsafe and risks stack buffer overflow because it does not check bounds when
concatenating strings into temp_buffer, which is only 128 bytes. Replace this
strcat with a bounds-checked string appender function that takes the destination
buffer size as a parameter and ensures no overflow occurs. Update all call sites
to initialize temp_buffer to an empty string and use this safe appender to
concatenate strings, passing the buffer size each time.
| void __attribute__((noreturn)) PanicFromInterrupt(const char* message, Registers* regs) { | ||
| PanicContext ctx = {0}; | ||
| ctx.rip = regs->rip; | ||
| ctx.rsp = regs->rsp; | ||
| ctx.rbp = regs->rbp; | ||
| ctx.error_code = regs->error_code; | ||
| ctx.function = "N/A (From Interrupt)"; | ||
| ctx.file = ""; | ||
| ctx.line = 0; | ||
| KernelPanicHandler(message, regs->error_code, &ctx); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Keep definition type spelling consistent and protect against null regs
Match the header’s struct spelling and add a defensive null check.
-void __attribute__((noreturn)) PanicFromInterrupt(const char* message, Registers* regs) {
+void __attribute__((noreturn)) PanicFromInterrupt(const char* message, struct Registers* regs) {
+ if (!regs) {
+ Panic("PanicFromInterrupt called with null regs");
+ }
PanicContext ctx = {0};
ctx.rip = regs->rip;
ctx.rsp = regs->rsp;
ctx.rbp = regs->rbp;
ctx.error_code = regs->error_code;
ctx.function = "N/A (From Interrupt)";
ctx.file = "";
ctx.line = 0;
KernelPanicHandler(message, regs->error_code, &ctx);
}🏁 Script executed:
#!/bin/bash
# Search for PanicFromInterrupt declaration and definition across the codebase
rg -n "PanicFromInterrupt" Length of output: 717
🏁 Script executed:
#!/bin/bash
# Inspect Panic.h to confirm declaration and includes
sed -n '1,200p' kernel/core/Panic.h
# Inspect Process.h to find struct Registers definition or typedef
sed -n '1,200p' kernel/core/Process.h
# Search for any typedef alias for Registers
rg -n "typedef.*Registers" -n kernelLength of output: 1866
🏁 Script executed:
#!/bin/bash
# Locate the file defining `struct Registers`
FILE=$(rg -l "struct Registers" -n kernel | head -n1)
if [ -z "$FILE" ]; then
echo "struct Registers definition not found"
exit 1
fi
echo "---- Definition found in: $FILE ----"
sed -n '1,200p' "$FILE"Length of output: 8451
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate the struct Registers definition in Process.h
rg -n "struct Registers" -n kernel/process/Process.h
# Check for typedef alias for struct Registers elsewhere in the repo
rg -n "typedef struct Registers" -n .
Length of output: 195
Match PanicFromInterrupt signature to its declaration and add a null‐check
The header declares:
void __attribute__((noreturn)) PanicFromInterrupt(const char* message, struct Registers* regs);so the implementation should use the same spelling and guard against a null pointer before dereferencing:
• kernel/core/Panic.c (around line 183)
- void __attribute__((noreturn)) PanicFromInterrupt(const char* message, Registers* regs) {
+ void __attribute__((noreturn)) PanicFromInterrupt(const char* message, struct Registers* regs) {
+ if (unlikely(!regs)) {
+ Panic("PanicFromInterrupt called with null regs");
+ }
PanicContext ctx = {0};
ctx.rip = regs->rip;
ctx.rsp = regs->rsp;
ctx.rbp = regs->rbp;
ctx.error_code = regs->error_code;
ctx.function = "N/A (From Interrupt)";
ctx.file = "";
ctx.line = 0;
KernelPanicHandler(message, regs->error_code, &ctx);
}🤖 Prompt for AI Agents
In kernel/core/Panic.c around lines 183 to 194, the function
PanicFromInterrupt's parameter type for regs should match the declaration by
using 'struct Registers*' instead of just 'Registers*'. Additionally, add a
null-check for the regs pointer at the start of the function to prevent
dereferencing a null pointer. If regs is null, handle the error appropriately or
return early before accessing its fields.
| void strcpy(char* dest, const char* src) { | ||
| while ((*dest++ = *src++)); | ||
| } | ||
|
|
||
| void strcat(char* dest, const char* src) { | ||
| while (*dest) dest++; | ||
| while ((*dest++ = *src++)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null-pointer assertions & standard-style return
Both strcpy and strcat run unchecked on dest/src. A simple ASSERT(dest && src); (as used elsewhere in MemOps) avoids catastrophic faults.
Also, returning char* (pointer to dest) matches the conventional prototype and allows call-chaining.
🤖 Prompt for AI Agents
In kernel/memory/MemOps.c around lines 5 to 12, add assertions to check that
both dest and src pointers are not null at the start of strcpy and strcat
functions using ASSERT(dest && src);. Then, change the return type of both
functions from void to char* and add a return statement returning the original
dest pointer at the end of each function to match the standard C library
prototypes and enable call-chaining.
| void strcpy(char* dest, const char* src); | ||
| void strcat(char* dest, const char* src); | ||
| void htoa(uint64_t n, char* buffer); | ||
| void itoa(uint64_t n, char* buffer); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose size-checked helpers or document required buffer sizes
strcpy / strcat with no length parameter invite silent buffer overruns in kernel space, and htoa/itoa require the caller to pass a ≥19-byte buffer but that contract isn’t documented here.
Consider:
-void strcpy(char* dest, const char* src);
-void strcat(char* dest, const char* src);
+size_t strlcpy(char* dest, const char* src, size_t dest_sz); /* returns bytes copied */
+size_t strlcat(char* dest, const char* src, size_t dest_sz);…or at least add explicit comments on minimum buffer sizes and behaviour.
📝 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.
| void strcpy(char* dest, const char* src); | |
| void strcat(char* dest, const char* src); | |
| void htoa(uint64_t n, char* buffer); | |
| void itoa(uint64_t n, char* buffer); | |
| size_t strlcpy(char* dest, const char* src, size_t dest_sz); /* returns bytes copied */ | |
| size_t strlcat(char* dest, const char* src, size_t dest_sz); | |
| void htoa(uint64_t n, char* buffer); | |
| void itoa(uint64_t n, char* buffer); |
🤖 Prompt for AI Agents
In kernel/memory/MemOps.h around lines 11 to 14, the functions strcpy and strcat
lack length parameters, risking buffer overruns, and htoa/itoa require a minimum
19-byte buffer which is not documented. To fix this, replace strcpy and strcat
with size-checked versions that take buffer length arguments, or add clear
comments specifying the required minimum buffer sizes and behavior for all these
functions to ensure safe usage.
1 participant
Summary by CodeRabbit
New Features
Improvements
Style
Bug Fixes
Chores