Development #22
Development #22
Conversation
WalkthroughA new inter-process communication (IPC) subsystem was introduced, including message structures, queue management, and kernel-level send/receive functions. System call support for IPC was added, with corresponding syscall numbers and handler integration. Numerous assertions for null-pointer safety were inserted throughout the kernel. The build system and process structures were updated to accommodate the new IPC features. Changes
Sequence Diagram(s)sequenceDiagram
participant UserProcessA
participant Kernel
participant UserProcessB
UserProcessA->>Kernel: Syscall(SYS_IPC_SEND, B_pid, &msg)
Kernel->>UserProcessB: Enqueue msg in B's IPC queue
UserProcessB->>Kernel: Syscall(SYS_IPC_RECV, &msg_buffer)
alt Message available
Kernel->>UserProcessB: Copy msg to msg_buffer
else Queue empty
Kernel->>UserProcessB: Block process until message arrives
end
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 5
🧹 Nitpick comments (4)
Kernel/Core/stdint.h (1)
8-9: Consider moving NULL definition to a more appropriate header.While the NULL definition is technically correct, placing it in
stdint.hdeviates from standard practice where NULL is typically defined instddef.horstdlib.h. This could lead to conflicts if standard library headers are later included.Consider creating a dedicated
stddef.hheader or moving this definition to a more appropriate location.Kernel/Core/Ipc.h (3)
6-6: Consider making message queue size configurable.The hardcoded limit of 16 messages per process may be too restrictive for some use cases.
Consider making this configurable per process or at compile time:
-#define MAX_MESSAGES 16 // Max messages per process queue +#ifndef MAX_MESSAGES +#define MAX_MESSAGES 16 // Default max messages per process queue +#endif
15-23: Consider adding message versioning and flexible sizing.The current design has limitations:
- Fixed 256-byte data payload may waste memory for small messages
- No support for larger messages
- No protocol versioning for future compatibility
Consider adding:
- A version field to the IpcMessage struct for future compatibility
- Support for variable-length messages using a separate shared memory mechanism for large payloads
- A flags field for message attributes (e.g., priority, delivery guarantees)
32-33: Add documentation for IPC functions.The public API functions lack documentation about their behavior, parameters, and return values.
Add function documentation:
+/** + * Send an IPC message to a target process + * @param target_pid The PID of the target process + * @param msg Pointer to the message to send (sender_pid will be set by kernel) + * @return 0 on success, negative error code on failure + */ void IpcSendMessage(uint32_t target_pid, IpcMessage* msg); + +/** + * Receive an IPC message (blocks if no messages available) + * @param msg_buffer Buffer to store the received message + * @return 0 on success, negative error code on failure + */ int IpcReceiveMessage(IpcMessage* msg_buffer);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Kernel/Core/Ipc.c(1 hunks)Kernel/Core/Ipc.h(1 hunks)Kernel/Core/Kernel.h(1 hunks)Kernel/Core/Panic.h(1 hunks)Kernel/Core/stdint.h(1 hunks)Kernel/Drivers/Cpu.c(1 hunks)Kernel/Drivers/Cpu.h(0 hunks)Kernel/Drivers/Interrupts.c(1 hunks)Kernel/Memory/MemOps.c(3 hunks)Kernel/Process/Process.c(4 hunks)Kernel/Process/Process.h(2 hunks)Kernel/Process/UserMode.c(2 hunks)Kernel/System/Syscall.c(2 hunks)Kernel/System/Syscall.h(1 hunks)Kernel/System/SyscallEntry.asm(2 hunks)meson.build(1 hunks)
💤 Files with no reviewable changes (1)
- Kernel/Drivers/Cpu.h
🧰 Additional context used
🧠 Learnings (3)
Kernel/Drivers/Interrupts.c (1)
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
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.
Kernel/System/Syscall.h (1)
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
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.
Kernel/System/Syscall.c (1)
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
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.
🧬 Code Graph Analysis (1)
Kernel/System/Syscall.c (3)
Kernel/System/Syscall.h (1)
Syscall(19-19)Kernel/Core/Ipc.c (2)
IpcSendMessage(6-28)IpcReceiveMessage(30-46)Kernel/Core/Ipc.h (2)
IpcSendMessage(32-32)IpcReceiveMessage(33-33)
🔇 Additional comments (22)
Kernel/Core/Panic.h (2)
10-11: LGTM: Proper macro expansion fix.The two-level macro expansion using
STRINGIFY_HELPERis the correct approach to ensure that macro arguments like__LINE__are properly expanded before stringification. This prevents the ASSERT macro from displaying "LINE" instead of the actual line number.
15-15: LGTM: ASSERT macro syntax correction.The ASSERT macro syntax appears properly corrected with appropriate parentheses matching for the Panic function call.
Kernel/Drivers/Interrupts.c (1)
71-71: LGTM: Appropriate defensive programming.The null pointer assertion for the
regsparameter is good defensive programming, especially since the function immediately dereferencesregs->interrupt_numberwithout validation. This prevents undefined behavior and provides clear error reporting if a null pointer is passed.Kernel/Drivers/Cpu.c (2)
2-2: LGTM: Necessary header inclusion.Including
Panic.his required for theASSERTmacro used in the function below.
6-6: LGTM: Comprehensive pointer validation.The assertion properly validates all four output pointers before the inline assembly instruction. This prevents undefined behavior when the
cpuidinstruction attempts to write to potentially null pointers.Kernel/Process/UserMode.c (2)
8-8: LGTM: Essential function pointer validation.The assertion ensures the
user_functionpointer is valid before it's used in the inline assembly for user mode transition. This prevents undefined behavior when jumping to a null function pointer.
36-36: LGTM: Consistent parameter validation.The assertion validates the
user_functionpointer before passing it toCreateProcess, maintaining consistency with the defensive programming pattern established throughout the kernel.Kernel/Memory/MemOps.c (4)
3-3: Good defensive programming practice.Adding the Panic.h include to support the ASSERT macros is appropriate for kernel-level defensive programming.
6-6: Verify performance impact of assertions in hot path functions.The null pointer assertion is good defensive programming, but these memory operation functions appear to be performance-critical (using SSE2 optimizations). Consider whether the assertion overhead is acceptable for these hot path functions.
39-39: Comprehensive null pointer validation.The assertion correctly validates both destination and source pointers before proceeding with the memory copy operation.
64-64: Appropriate assertion for page operations.The null pointer check is essential for page-level operations where a null pointer could cause significant system instability.
Kernel/Core/Kernel.h (1)
3-3: Appropriate include for fixed-width integer types.Adding stdint.h support is reasonable for the IPC functionality and provides consistent integer type definitions across the kernel.
meson.build (1)
66-66: Proper integration of IPC source into build system.The addition of the IPC source file follows the existing build pattern and ensures the new functionality is properly compiled and linked.
Kernel/System/SyscallEntry.asm (1)
3-3: Approved: Syscall name matches C implementationVerified that
Syscallis declared in Kernel/System/Syscall.h and defined in Kernel/System/Syscall.c, with no remainingSyscallHandlerreferences.
All naming is consistent and aligned with the C implementation.Kernel/Process/Process.h (2)
5-5: Appropriate IPC header inclusion.The include path for the IPC header is correct and necessary for the MessageQueue type definition.
52-52: IPC queue initialization confirmedThe
ipc_queuemember is properly initialized inCreateSecureProcess:
- In
Kernel/Process/Process.c, after zeroing the new process slot withFastMemset(&processes[slot], 0, sizeof(Process));, the code explicitly sets:
processes[slot].ipc_queue.head = 0;processes[slot].ipc_queue.tail = 0;processes[slot].ipc_queue.count = 0;No further changes are needed.
Kernel/Process/Process.c (3)
7-7: LGTM!The IPC header inclusion is necessary for the new IPC subsystem integration.
179-183: Proper IPC queue initialization.Good practice to explicitly initialize the IPC queue fields to ensure a clean state for new processes.
205-205: Good defensive programming.Adding the null assertion prevents potential crashes from null pointer dereference.
Kernel/System/Syscall.h (2)
11-12: IPC syscall definitions look good.The new syscall numbers for IPC send and receive operations are properly defined.
19-19: Function rename is consistent.The Syscall function declaration matches the implementation rename.
Kernel/System/Syscall.c (1)
7-7: Required include for IPC functionality.The IPC header inclusion is necessary for the syscall handlers.
|
|
||
| void ProcessExitStub() { | ||
| PrintKernel("[KERNEL] Process returned from its main function. This is an error!\n"); | ||
| PrintKernel("[KERNEL] Process returned from its main function. -FATAL EXECPTION-\n"); |
There was a problem hiding this comment.
Fix typo in error message.
"EXECPTION" should be "EXCEPTION".
- PrintKernel("[KERNEL] Process returned from its main function. -FATAL EXECPTION-\n");
+ PrintKernel("[KERNEL] Process returned from its main function. -FATAL EXCEPTION-\n");📝 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.
| PrintKernel("[KERNEL] Process returned from its main function. -FATAL EXECPTION-\n"); | |
| PrintKernel("[KERNEL] Process returned from its main function. -FATAL EXCEPTION-\n"); |
🤖 Prompt for AI Agents
In Kernel/Process/Process.c at line 123, correct the typo in the error message
by changing "EXECPTION" to "EXCEPTION" in the PrintKernel call to ensure the
message is spelled correctly.
| case SYS_IPC_SEND: | ||
| // arg1 = target_pid, arg2 = message | ||
| IpcSendMessage((uint32_t)arg1, (IpcMessage*)arg2); | ||
| return 0; |
There was a problem hiding this comment.
Critical: Validate user pointers and add security checks for IPC send.
The current implementation has several security vulnerabilities:
- No validation of the user-provided message pointer
- No bounds checking on message content
- No authorization checks for sending messages to other processes
Consider implementing proper validation:
case SYS_IPC_SEND:
// arg1 = target_pid, arg2 = message
- IpcSendMessage((uint32_t)arg1, (IpcMessage*)arg2);
- return 0;
+ if (!arg2 || arg1 == 0) {
+ return -1; // Invalid arguments
+ }
+
+ // Copy message from user space to kernel space
+ IpcMessage kernel_msg;
+ if (CopyFromUser(&kernel_msg, (void*)arg2, sizeof(IpcMessage)) != 0) {
+ return -1; // Failed to copy from user space
+ }
+
+ // Validate message fields
+ if (kernel_msg.size > sizeof(kernel_msg.payload.data)) {
+ return -1; // Invalid message size
+ }
+
+ // Set sender PID to prevent spoofing
+ kernel_msg.sender_pid = current->pid;
+
+ IpcSendMessage((uint32_t)arg1, &kernel_msg);
+ return 0;Note: You'll need to implement CopyFromUser function for safe user-to-kernel memory copying.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Kernel/System/Syscall.c around lines 54 to 57, the code lacks validation of
the user-provided message pointer, bounds checking on the message content, and
authorization checks for sending IPC messages. To fix this, implement a
CopyFromUser function to safely copy the message from user space to kernel
space, validate that the message pointer is not null and points to accessible
memory, check the message size against allowed limits, and verify that the
sending process is authorized to send messages to the target process before
calling IpcSendMessage.
| case SYS_IPC_RECV: | ||
| // arg1 = message_buffer | ||
| return IpcReceiveMessage((IpcMessage*)arg1); |
There was a problem hiding this comment.
Critical: Validate user buffer for IPC receive.
The implementation lacks validation of the user-provided buffer pointer, which could lead to arbitrary memory writes.
Implement proper validation and safe copying:
case SYS_IPC_RECV:
// arg1 = message_buffer
- return IpcReceiveMessage((IpcMessage*)arg1);
+ if (!arg1) {
+ return -1; // Invalid buffer
+ }
+
+ // Receive message into kernel buffer first
+ IpcMessage kernel_msg;
+ int result = IpcReceiveMessage(&kernel_msg);
+ if (result != 0) {
+ return result;
+ }
+
+ // Copy message to user space
+ if (CopyToUser((void*)arg1, &kernel_msg, sizeof(IpcMessage)) != 0) {
+ return -1; // Failed to copy to user space
+ }
+
+ return 0;Note: You'll need to implement CopyToUser function for safe kernel-to-user memory copying.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Kernel/System/Syscall.c around lines 59 to 61, the code directly uses the
user-provided buffer pointer for IPC receive without validating it, risking
arbitrary memory writes. To fix this, add validation to ensure the user buffer
pointer is valid and accessible before use. Implement and use a CopyToUser
function to safely copy data from kernel space to the user buffer, preventing
unsafe memory operations.
| int IpcReceiveMessage(IpcMessage* msg_buffer) { | ||
| ASSERT(msg_buffer != NULL); | ||
|
|
||
| Process* current = GetCurrentProcess(); | ||
| MessageQueue* queue = ¤t->ipc_queue; | ||
|
|
||
| while (queue->count == 0) { | ||
| current->state = PROC_BLOCKED; | ||
| Yield(); | ||
| } | ||
|
|
||
| FastMemcpy(msg_buffer, &queue->messages[queue->head], sizeof(IpcMessage)); | ||
| queue->head = (queue->head + 1) % MAX_MESSAGES; | ||
| queue->count--; | ||
|
|
||
| return 0; // Success | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add synchronization and improve blocking mechanism.
The current implementation has potential race conditions and inefficient blocking:
Consider these improvements:
int IpcReceiveMessage(IpcMessage* msg_buffer) {
ASSERT(msg_buffer != NULL);
Process* current = GetCurrentProcess();
MessageQueue* queue = ¤t->ipc_queue;
+ // TODO: Add spinlock or disable interrupts here for queue protection
+
while (queue->count == 0) {
+ // TODO: Release spinlock before blocking
current->state = PROC_BLOCKED;
Yield();
+ // TODO: Re-acquire spinlock after waking up
}
FastMemcpy(msg_buffer, &queue->messages[queue->head], sizeof(IpcMessage));
queue->head = (queue->head + 1) % MAX_MESSAGES;
queue->count--;
+
+ // TODO: Release spinlock or re-enable interrupts here
return 0; // Success
}Also consider adding a timeout mechanism to prevent indefinite blocking.
📝 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.
| int IpcReceiveMessage(IpcMessage* msg_buffer) { | |
| ASSERT(msg_buffer != NULL); | |
| Process* current = GetCurrentProcess(); | |
| MessageQueue* queue = ¤t->ipc_queue; | |
| while (queue->count == 0) { | |
| current->state = PROC_BLOCKED; | |
| Yield(); | |
| } | |
| FastMemcpy(msg_buffer, &queue->messages[queue->head], sizeof(IpcMessage)); | |
| queue->head = (queue->head + 1) % MAX_MESSAGES; | |
| queue->count--; | |
| return 0; // Success | |
| } | |
| int IpcReceiveMessage(IpcMessage* msg_buffer) { | |
| ASSERT(msg_buffer != NULL); | |
| Process* current = GetCurrentProcess(); | |
| MessageQueue* queue = ¤t->ipc_queue; | |
| // TODO: Add spinlock or disable interrupts here for queue protection | |
| while (queue->count == 0) { | |
| // TODO: Release spinlock before blocking | |
| current->state = PROC_BLOCKED; | |
| Yield(); | |
| // TODO: Re-acquire spinlock after waking up | |
| } | |
| FastMemcpy(msg_buffer, &queue->messages[queue->head], sizeof(IpcMessage)); | |
| queue->head = (queue->head + 1) % MAX_MESSAGES; | |
| queue->count--; | |
| // TODO: Release spinlock or re-enable interrupts here | |
| return 0; // Success | |
| } |
🤖 Prompt for AI Agents
In Kernel/Core/Ipc.c around lines 30 to 46, the IpcReceiveMessage function lacks
synchronization, leading to potential race conditions when accessing the message
queue, and uses a busy-wait loop for blocking. To fix this, introduce proper
synchronization primitives like mutexes or spinlocks to protect access to the
queue, replace the busy-wait with a blocking wait mechanism such as condition
variables or semaphores to efficiently block the process until a message
arrives, and optionally implement a timeout feature to avoid indefinite blocking
by returning an error or timeout status if no message is received within the
specified time.
| void IpcSendMessage(uint32_t target_pid, IpcMessage* msg) { | ||
| ASSERT(msg != NULL); | ||
|
|
||
| Process* target = GetProcessByPid(target_pid); | ||
| if (!target) { | ||
| // Handle error: target process not found | ||
| return; | ||
| } | ||
|
|
||
| MessageQueue* queue = &target->ipc_queue; | ||
| if (queue->count >= MAX_MESSAGES) { | ||
| // Handle error: queue is full | ||
| return; | ||
| } | ||
|
|
||
| FastMemcpy(&queue->messages[queue->tail], msg, sizeof(IpcMessage)); | ||
| queue->tail = (queue->tail + 1) % MAX_MESSAGES; | ||
| queue->count++; | ||
|
|
||
| if (target->state == PROC_BLOCKED) { | ||
| target->state = PROC_READY; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add synchronization for IpcSendMessage.
Several issues need to be addressed:
- Silent failures on lines 12 and 18 make debugging difficult
- No synchronization primitives protecting queue access (race conditions possible)
- The sender_pid should be set by the kernel to prevent spoofing
Consider these improvements:
-void IpcSendMessage(uint32_t target_pid, IpcMessage* msg) {
+int IpcSendMessage(uint32_t target_pid, IpcMessage* msg) {
ASSERT(msg != NULL);
Process* target = GetProcessByPid(target_pid);
if (!target) {
- // Handle error: target process not found
- return;
+ return -ESRCH; // No such process
}
MessageQueue* queue = &target->ipc_queue;
+
+ // TODO: Add spinlock or disable interrupts here for queue protection
+
if (queue->count >= MAX_MESSAGES) {
- // Handle error: queue is full
- return;
+ return -EAGAIN; // Resource temporarily unavailable
}
+
+ // Set sender PID to prevent spoofing
+ msg->sender_pid = GetCurrentProcess()->pid;
FastMemcpy(&queue->messages[queue->tail], msg, sizeof(IpcMessage));
queue->tail = (queue->tail + 1) % MAX_MESSAGES;
queue->count++;
+
+ // TODO: Release spinlock or re-enable interrupts here
if (target->state == PROC_BLOCKED) {
target->state = PROC_READY;
}
+
+ return 0; // Success
}📝 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 IpcSendMessage(uint32_t target_pid, IpcMessage* msg) { | |
| ASSERT(msg != NULL); | |
| Process* target = GetProcessByPid(target_pid); | |
| if (!target) { | |
| // Handle error: target process not found | |
| return; | |
| } | |
| MessageQueue* queue = &target->ipc_queue; | |
| if (queue->count >= MAX_MESSAGES) { | |
| // Handle error: queue is full | |
| return; | |
| } | |
| FastMemcpy(&queue->messages[queue->tail], msg, sizeof(IpcMessage)); | |
| queue->tail = (queue->tail + 1) % MAX_MESSAGES; | |
| queue->count++; | |
| if (target->state == PROC_BLOCKED) { | |
| target->state = PROC_READY; | |
| } | |
| } | |
| int IpcSendMessage(uint32_t target_pid, IpcMessage* msg) { | |
| ASSERT(msg != NULL); | |
| Process* target = GetProcessByPid(target_pid); | |
| if (!target) { | |
| return -ESRCH; // No such process | |
| } | |
| MessageQueue* queue = &target->ipc_queue; | |
| // TODO: Add spinlock or disable interrupts here for queue protection | |
| if (queue->count >= MAX_MESSAGES) { | |
| return -EAGAIN; // Resource temporarily unavailable | |
| } | |
| // Set sender PID to prevent spoofing | |
| msg->sender_pid = GetCurrentProcess()->pid; | |
| FastMemcpy(&queue->messages[queue->tail], msg, sizeof(IpcMessage)); | |
| queue->tail = (queue->tail + 1) % MAX_MESSAGES; | |
| queue->count++; | |
| // TODO: Release spinlock or re-enable interrupts here | |
| if (target->state == PROC_BLOCKED) { | |
| target->state = PROC_READY; | |
| } | |
| return 0; // Success | |
| } |
🤖 Prompt for AI Agents
In Kernel/Core/Ipc.c around lines 6 to 28, improve IpcSendMessage by replacing
silent returns on errors with explicit error logging or handling to aid
debugging, add synchronization (e.g., a mutex or spinlock) around the message
queue access to prevent race conditions, and ensure the sender_pid field in the
IpcMessage is set by the kernel within this function before copying the message
to prevent sender spoofing.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores