Out-of-Line memory deallocation: MIG vs. glibc

[olafBuddenhagen]

Hi,

A while back, Michael observed that building the debian package of
zaptel regularily causes the FS in the build chroot to crash. I could
reproduce that on my system, and tried to hunt down the problem.

I reduced the testcase to simply

   fakeroot-tcp sh -c $suid_binary

Most of the time this will make the FS on which $suid_binary resides
crash. ($suid_binary is "ps" in case of zaptel, but it also works with
other suid binaries like "ping".)

Interestingly, I observed already a long time ago that running "ps" in a
loop considerably destabilizes the system -- it seems to be the very
same problem. (Only it's harder to track down in that case, as it
happens stochastically...)

Debugging the thing, I made three relevant observations:

1. With the "fakeroot-tcp sh -c" case, the file_exec call receives an
enormous amount of several hundred port rights for the fdarray[]
argument (as well as some others) -- so many that they are automatically
transferred out-of-line, which is one of the conditions causing the
crash. I wonder whether this is some bug by itself.

2. exec_exec() (called from fs_S_file_exec) on suid binaries always
returns EINTR. (Unless it doesn't return at all, which sometimes
happens, avoiding the crash...) For normal binaries, success is
returned. Again, I wonder whether this is expected behaviour, or a bug.

3. Given the above conditions (RPC request message has port rights
transferred out-of-line, and an error return code is generated), the FS
will crash inside glibc.

Looking at the code, this must be so: __mach_msg_server_timeout() (in
$glibc/mach/msgserver.c) does a __mach_msg_destroy() on the Request
message whenever an error was returned (from the demuxer).
__mach_msg_destroy() tries to deallocate all port rigths and all
out-of-line memory in the message. However, at the end of the
MIG-generated server routine (_Xfile_exec() from
$hurd/build/libdiskfs/fsServer.c in our case), any out-of-line memory
from the request message is already deallocated. This is no problem for
out-of-line data (double free doesn't seem to hurt), but if there are
port rights transferred out-of-line, this will result in
__mach_msg_destroy() trying to access the port rights in the already
freed memory -- resulting in a "bad memory access" exception.

This third observation is obviosly a bug, which could show in other
situations as well. (Whenever an RPC sends a large amount of port rights
and the server returns some error.)

I'm not sure what is The Right Way (TM) to fix this. The only one I
could come up, is for the MIG-generated code to skip the memory
deallocation if an error code is returned; but I'm not sure this can be
considered correct behaviour.

Nonetheless, I whacked up a MIG patch which makes it do exactly that.
Hurd rebuilt with this patched MIG isn't vulnerable to "fakeroot-tcp sh
-c $suid_binary" any more, and so far I haven't observed any ill effects
caused by the change. (Haven't tried rebuilding glibc with the patched
MIG, though -- I don't have enough disk space for that :-( )


2006-04-09  Olaf Buddenhagen  <antrik@gmx.net>

        * server.c (WriteDestroyArg): Only dealloc out-of-line memory from
        request message if KERN_SUCCESS

diff -ur mig-1.3.1_orig/server.c mig-1.3.1/server.c
--- mig-1.3.1_orig/server.c     1999-10-11 10:44:49.000000000 +0200
+++ mig-1.3.1/server.c  2006-04-09 17:57:35.000000000 +0200
@@ -761,16 +761,21 @@
     if (akCheck(arg->argKind, akbIndefinite)) {
        /*
         * Deallocate only if out-of-line.
+        *
+        * Also skip deallocation if error occured in processing the RPC.
+        * (Generic RPC handling code will clean up in this case)
         */
        argument_t *count = arg->argCount;
        ipc_type_t *btype = it->itElement;
        int     multiplier = btype->itTypeSize / btype->itNumber;
 
-       fprintf(file, "\tif (!In%dP->%s%s.msgt_inline)\n",
+       fprintf(file, "\tif (OutP->%s == KERN_SUCCESS)\n",
+               arg->argRoutine->rtRetCode->argMsgField);
+       fprintf(file, "\t\tif (!In%dP->%s%s.msgt_inline)\n",
                arg->argRequestPos,
                arg->argTTName,
                arg->argLongForm ? ".msgtl_header" : "");
-       fprintf(file, "\t\t%smig_deallocate(* (vm_offset_t *) %s, ",
+       fprintf(file, "\t\t\t%smig_deallocate(* (vm_offset_t *) %s, ",
                SubrPrefix, InArgMsgField(arg));
        if (multiplier > 1)
            fprintf(file, "%d * ", multiplier);


-antrik-