Bug#102437: dealloc analysis for each server function, more bugs!

[Marcus Brinkmann]

On Thu, Jul 12, 2001 at 04:34:45PM -0400, Roland McGrath wrote:
> I didn't look closely, but now I am thinking twice.  If the data is small
> enough to fit inline, and you use a separate reply stub, then you can send
> it without copying.  That ought to be the common case.

This would be what you want.  I can't say I like it.  Maybe if we were to
use a seperate reply stub in auth_getids as well, we might use the same code
again.  Otherwise I am afraid we have two slightly different versions of the
macros.  (The argument why to use a seperate reply stub for auth_getids is
the same as for auth_server_authenticate, we can avoid a copy).

Thanks,
Marcus

--- auth.c      Thu Jul 12 22:52:53 2001
+++ auth.c.new  Thu Jul 12 23:11:05 2001
@@ -27,6 +27,7 @@
 #include <hurd/ports.h>
 #include <hurd/ihash.h>
 #include <idvec.h>
+#include <sys/mman.h>
 #include <assert.h>
 #include <argp.h>
 #include <error.h>
@@ -82,18 +83,57 @@ auth_port_to_handle (auth_t auth)
 
 /* id management.  */
 
-inline void idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
+inline int idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
 {
   if (idvec->num > *nids)
-    *ids = idvec->ids;
+    {
+      *ids = mmap (0, idvec->num * sizeof (**ids), PROT_READ|PROT_WRITE,
+                  MAP_ANON, 0, 0);
+      if (*ids == (uid_t *) -1)
+       return errno;
+    }
+  memcpy (*ids, idvec->ids, idvec->num * sizeof (**ids));
+  *nids = idvec->num;
+
+  return 0;
+}
+
+#define C(auth, ids)   if (! err) \
+                         err = idvec_copyout (&auth->ids, ids, n##ids)
+#define OUTIDS(auth)   { C (auth, euids); C (auth, egids); \
+                         C (auth, auids); C (auth, agids); }
+#define D(ids) *ids##_saved = *ids
+#define SAVEIDS        uid_t D (euids), D (egids), D (auids), D (agids)
+#define F(ids) if (*ids != ids##_saved) \
+                 munmap (*ids, *n##ids * sizeof (uid_t))
+#define FREEIDS { F (euids); F (egids); F (auids); F (agids); }
+
+/* In auth_server_authenticate, we use a reply stub to avoid copying
+   small inlined data twice.  */
+inline int idvec_copyout_reply (struct idvec *idvec, uid_t **ids, uid_t *nids)
+{
+  if (idvec->num > *nids)
+    {
+      *ids = mmap (0, idvec->num * sizeof (**ids), PROT_READ|PROT_WRITE,
+                  MAP_ANON, 0, 0);
+      if (*ids == (uid_t *) -1)
+       return errno;
+      memcpy (*ids, idvec->ids, idvec->num * sizeof (**ids));
+    }
   else
-    memcpy (*ids, idvec->ids, idvec->num * sizeof *ids);
+    *ids = idvec->ids;
+
   *nids = idvec->num;
+  return 0;
 }
 
-#define C(auth, ids)   idvec_copyout (&auth->ids, ids, n##ids)
-#define OUTIDS(auth)   (C (auth, euids), C (auth, egids), \
-                        C (auth, auids), C (auth, agids))
+#define C_R(auth, ids) if (! err) \
+                         err = idvec_copyout_reply (&auth->ids, ids, n##ids)
+#define OUTIDS_R(auth) { C_R (auth, euids); C_R (auth, egids); \
+                         C_R (auth, auids); C_R (auth, agids); }
+#define F_R(idsr)      if (*idsr != user->idsr.ids) \
+                         munmap (*idsr, *n##idsr * sizeof (uid_t))
+#define FREEIDS_R { F_R (euids); F_R (egids); F_R (auids); F_R (agids); }
 
 /* Implement auth_getids as described in <hurd/auth.defs>. */
 kern_return_t
@@ -107,12 +147,17 @@ S_auth_getids (struct authhandle *auth,
               uid_t **agids,
               u_int *nagids)
 {
+  int err = 0;
+  SAVEIDS;
+
   if (! auth)
     return EOPNOTSUPP;
-
+  
   OUTIDS (auth);
-
-  return 0;
+  if (err)
+    FREEIDS;
+      
+  return err;
 }
 
 /* Implement auth_makeauth as described in <hurd/auth.defs>. */
@@ -358,6 +403,7 @@ S_auth_server_authenticate (struct authh
                            uid_t **agids,
                            u_int *nagids)
 {
+  error_t err = 0;
   struct pending *u;
   struct authhandle *user;
 
@@ -392,7 +438,6 @@ S_auth_server_authenticate (struct authh
       /* No pending user RPC for this port.
         Create a pending server RPC record.  */
       struct pending s;
-      error_t err;
 
       err = ihash_add (pending_servers, rendezvous, &s, &s.locp);
       if (! err)
@@ -418,17 +463,21 @@ S_auth_server_authenticate (struct authh
       user = s.user;
     }
 
-  /* Extract the ids.  We must use a separate reply stub so
-     we can deref the user auth handle after the reply uses its
-     contents.  */
+  OUTIDS_R (user);
+  if (err)
+    FREEIDS_R;
+
+  /* Extract the ids.  We use a separate reply stub to avoid copying
+     small data twice.  For this to work we must keep the user reference
+     until the reply stub did its work.  */
   auth_server_authenticate_reply (reply, reply_type, 0,
-                                 user->euids.ids, user->euids.num,
-                                 user->auids.ids, user->auids.num,
-                                 user->egids.ids, user->egids.num,
-                                 user->agids.ids, user->agids.num);
+                                  *euids, *neuids,
+                                  *auids, *nauids,
+                                  *egids, *negids,
+                                  *agids, *nagids);
   ports_port_deref (user);
   mach_port_deallocate (mach_task_self (), rendezvous);
-  return MIG_NO_REPLY;
+  return err;
 }
 
 

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de