httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cliff Woolley <jwool...@virginia.edu>
Subject [PATCH] Re: the most common seg fault on daedalus
Date Mon, 08 Apr 2002 18:33:21 GMT
On Mon, 8 Apr 2002, Cliff Woolley wrote:

> Ha. ;-)  Actually I think I see a number of potential problems in
> apr/mmap/unix/mmap.c.  I'm working on a patch and I'll post it here.

Okay, there are two or three issues or potential issues addressed here.

(1) apr_mmap_dup() would register a cleanup on the *new_mmap even if the
new one was not the owner.  That turned out to be okay though because
mmap_cleanup would detect the !mm->is_owner case and just pretend
everything was cool.  It's just extra work that doesn't need to be done.

(2) In mmap_cleanup(), if munmap() fails for some reason, we would not set
mm->mm to -1, meaning if we ever tried to run the cleanup a second time
for some reason, we would not know we'd already tried to munmap() the
thing and we'd do it again.  This is the same kind of bug that killed us a
while back on directory cleanups if you'll recall.

(3) Finally (and this is, I believe, the source of our current segfaults):
okay, here's a weird sequence of events:

   - create an mmap
   - dup it (with or without transferring ownership)
   - cleanup or delete one copy
   - delete the other one

Because apr_mmap_delete() would assume that any APR_SUCCESS from
mmap_cleanup() meant "OK, go ahead and kill the cleanup", and
!mm->is_owner would return APR_SUCCESS, we would kill the cleanup even if
there wasn't one to kill.  Boom.  There are two or three related sequences
of events that cause this, but they all boil down to essentially the
above.

There are parallel problems in the win32 mmap code that will need patching
as well.

As a side note, the buckets code is okay because (and it even has a
comment to this effect), it assumes that apr_mmap_delete() will do the
Right Thing, and if the mmap is not owned or already deleted, it will just
be a no-op.

Greg, can you try this patch on daedalus for me? (sorry if there were line
wraps)

Thanks,
Cliff



Index: mmap.c
===================================================================
RCS file: /home/cvs/apr/mmap/unix/mmap.c,v
retrieving revision 1.40
diff -u -d -r1.40 mmap.c
--- mmap.c      13 Mar 2002 20:39:24 -0000      1.40
+++ mmap.c      8 Apr 2002 18:26:36 -0000
@@ -85,25 +85,21 @@
     apr_mmap_t *mm = themmap;
     int rv;

-    if (!mm->is_owner) {
-        return APR_SUCCESS;
+    if ((!mm->is_owner) || (mm->mm == (void *)-1)) {
+        /* XXX: we shouldn't ever get here */
+        return APR_ENOENT;
     }

 #ifdef BEOS
     rv = delete_area(mm->area);
-
-    if (rv == 0) {
-        mm->mm = (void *)-1;
-        return APR_SUCCESS;
-    }
 #else
     rv = munmap(mm->mm, mm->size);
+#endif
+    mm->mm = (void *)-1;

     if (rv == 0) {
-        mm->mm = (void *)-1;
         return APR_SUCCESS;
     }
-#endif
     return errno;
 }

@@ -189,24 +185,21 @@
             (*new_mmap)->is_owner = 1;
             old_mmap->is_owner = 0;
             apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
+            apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
+                                      apr_pool_cleanup_null);
         }
         else {
             (*new_mmap)->is_owner = 0;
         }
-        apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
-                                  apr_pool_cleanup_null);
     }
     return APR_SUCCESS;
 }

 APR_DECLARE(apr_status_t) apr_mmap_delete(apr_mmap_t *mm)
 {
-    apr_status_t rv;
+    apr_status_t rv = APR_SUCCESS;

-    if (mm->mm == (void *)-1)
-        return APR_ENOENT;
-
-    if ((rv = mmap_cleanup(mm)) == APR_SUCCESS) {
+    if (mm->is_owner && ((rv = mmap_cleanup(mm)) == APR_SUCCESS)) {
         apr_pool_cleanup_kill(mm->cntxt, mm, mmap_cleanup);
         return APR_SUCCESS;
     }


--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA



Mime
View raw message