Subject: Re: [PATCH] ipc_kmsg.c / ipc_mqueue.c - follow-up after review Hi all,
Sorry for the delay, time zones got me on this one. First off, thanks to Samuel and Bradley (Anti-AI) for the blunt and constructive review - you both pointed at exactly the right spots. I went back through both patches line by line against your comments: - Bradley, you were right about ipc_kmsg.c: the integer-overflow -> heap-overflow scenario doesn't hold up. IKM_EXPAND_FACTOR is 1 or 2 in practice, and copyinmsg() already bounds the copy against kmsg->ikm_size, so there's no real OOB write there. I stripped that whole thing out. The only real issue was the missing NULL check in ipc_kmsg_free(), and instead of silently swallowing it I just added an assert(kmsg != IKM_NULL), matching how the rest of the file already handles this kind of precondition. That's the whole patch now, 5 lines. - For ipc_mqueue.c, once I went through your comments (hiding bugs, bypassing ip_qlimit, mangled indentation, the no-op state reassignment), I couldn't find anything left worth keeping. So that patch is just a revert back to the original ipc_mqueue_send - no functional change proposed there. Both patches attached. Thanks again for catching this before it went further, Alperen On Sat, Jul 25, 2026 at 1:01 AM Samuel Thibault <[email protected]> wrote: > Hello, > > > diff --git a/gnumach/ipc/ipc_mqueue.c b/gnumach/ipc/ipc_mqueue.c > > index f770afb..834eb16 100644 > > --- a/gnumach/ipc/ipc_mqueue.c > > +++ b/gnumach/ipc/ipc_mqueue.c > > @@ -28,11 +27,11 @@ > > * the rights to redistribute these changes. > > */ > > /* > > - * File: ipc/ipc_mqueue.c > > - * Author: Rich Draves > > - * Date: 2026 > > + * File: ipc/ipc_mqueue.c > > + * Author: Rich Draves / Alperen ERKAN > > + * Date: 2026 > > @@ -156,6 +159,8 @@ ipc_mqueue_changed( > > > > /* > > * Routine: ipc_mqueue_send > > + * Author : Alperen ERKAN > > + * 2026 > > * Purpose: > > * Send a message to a port. The message holds a reference > > * for the destination port in the msgh_remote_port field. > > A couple lines is not copyrightable and does make you sole author of > the function. > > > @@ -52,6 +51,10 @@ > > #include <ipc/ipc_space.h> > > #include <ipc/ipc_marequest.h> > > > > +/* > > + * An absolute message queue upper limit to prevent OOM and memory > exhaustion. > > + */ > > +#define IPC_MQUEUE_HARD_LIMIT 65536 > > If we start adding absolute constraints on sizes etc. we'd want to put > that into a header dedicated for that, so people know where to tune > them. > > > @@ -180,8 +185,16 @@ ipc_mqueue_send( > > { > > ipc_port_t port; > > > > + /* Defensive C: Checking the incoming message object and > destination port */ > > + if (kmsg == IKM_NULL) { > > + return MACH_SEND_INVALID_DATA; > > + } > > Again, hiding bugs. > > > port = (ipc_port_t) kmsg->ikm_header.msgh_remote_port; > > - assert(IP_VALID(port)); > > + > > + if (!IP_VALID(port)) { > > + return MACH_SEND_INVALID_DEST; > > + } > > If userland passed a bogus port it should have been tested way before > this. > > > @@ -230,17 +243,17 @@ ipc_mqueue_send( > > } > > > > /* > > - * Don't block if: > > - * 1) We're under the queue limit. > > - * 2) Caller used the MACH_SEND_ALWAYS internal > option. > > - * 3) Message is sent to a send-once right. > > - */ > > - > > - if ((port->ip_msgcount < port->ip_qlimit) || > > - (option & MACH_SEND_ALWAYS) || > > - (MACH_MSGH_BITS_REMOTE(kmsg->ikm_header.msgh_bits) == > > - > MACH_MSG_TYPE_PORT_SEND_ONCE)) > > - break; > > + * Don't block if: > > + * 1) We're under the queue limit. > > + * 2) Caller used MACH_SEND_ALWAYS but we are under the hard > safety limit. > > + * 3) Message is sent to a send-once right. > > + */ > > Avoid mangling the existing content. > > > + > > + if ((port->ip_msgcount < port->ip_qlimit) || > > + ((option & MACH_SEND_ALWAYS) && (port->ip_msgcount < > IPC_MQUEUE_HARD_LIMIT)) || > > ? No, you are making ipc_mqueue_send ignore the port->ip_qlimit. > > > + (MACH_MSGH_BITS_REMOTE(kmsg->ikm_header.msgh_bits) == > > + MACH_MSG_TYPE_PORT_SEND_ONCE)) > > + break; > > > > /* must block waiting for queue to clear */ > > > > @@ -263,45 +276,49 @@ ipc_mqueue_send( > > counter(c_ipc_mqueue_send_block++); > > thread_block(thread_no_continuation); > > ip_lock(port); > > - > > + > > /* why did we wake up? */ > > > > - if (self->ith_state == MACH_MSG_SUCCESS) > > - continue; > > - assert(self->ith_state == MACH_SEND_IN_PROGRESS); > > + if (self->ith_state == MACH_MSG_SUCCESS) { > > + self->ith_state = MACH_MSG_SUCCESS; // veya temiz durum > sıfırlaması > > Always English, please. And this does not do anything. > > > + continue; > > + } > > + assert(self->ith_state == MACH_SEND_IN_PROGRESS); > > Again avoid mangling the indentation. > > > > > - /* take ourselves off blocked queue */ > > + /* take ourselves off blocked queue under port lock */ > > + ipc_thread_rmqueue(&port->ip_blocked, self); > > + > > + /* [DEFENSIVE]: Clear state to prevent stale status reuse */ > > + self->ith_state = MACH_MSG_SUCCESS; > > > > - ipc_thread_rmqueue(&port->ip_blocked, self); > > + /* > > + * Thread wakeup-reason field tells us why > > + * the wait was interrupted. > > + */ > > > > - /* > > - * Thread wakeup-reason field tells us why > > - * the wait was interrupted. > > - */ > > + switch (self->ith_wait_result) { > > + case THREAD_INTERRUPTED: > > + /* send was interrupted - give up */ > > This is completely mangled. No idea if there is anything to read here. > > Samuel >
0001-ipc_kmsg-assert-non-null-kmsg-in-ipc_kmsg_free.patch
Description: Binary data
0001-revert-ipc_mqueue_send-bogus-patch.patch
Description: Binary data
