[MPlayer-dev-eng] [PATCH] (loader/ext.c) VirtualAlloc() SIGSEGV on Linux + other fixes

[A. Guru]

* On Friday 2004-08-20 at 00:16:40 +0100, Martin Simmons wrote:
> Yes, I agree.  Your solution of removing MAP_FIXED is neater, provided the OS
> can be trusted to return the requested address.  It looks like it can on Linux
> 2.4.x kernels at least.

Cool.



There is a more efficient long term solution that would be optimal.
All the various projects that need to implement a win32-compatible
VirtualAlloc() on Linux, such as mplayer and wine (and xine?) (and
avifile?), would benefit from lobbying on the Linux kernel mailing list
for this minimal little patch:

========================================================================
--- linux-2.4.27/mm/mmap.c.orig-2.4.27	2004-02-18 08:36:32 -0500
+++ linux-2.4.27/mm/mmap.c	2004-08-16 09:29:10 -0400
@@ -490,7 +490,9 @@ unsigned long do_mmap_pgoff(struct file 
 munmap_back:
 	vma = find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent);
 	if (vma && vma->vm_start < addr + len) {
+		if (flags & MAP_NOUNMAP)
+			return -EINVAL;
 		if (do_munmap(mm, addr, len))
 			return -ENOMEM;
 		goto munmap_back;
 	}
========================================================================

The patch works for 2.4 as well as 2.6.  It is incomplete because it
omits the macroprocessor #defines for the new MAP_NOUNMAP flag on all the
different architectures that Linux supports.  Completing it is tediously
boring but simple; I will not personally do it until various projects
commit to pushing such a Linux kernel patch, together.

The addition of such an MAP_NOUNMAP flag could also be pushed on other
operating systems that implement MAP_FIXED.



For the VirtualAlloc() implementation in mplayer, this would mean doing
something like (on top of my current patch as the top of this thread):

========================================================================
--- loader/ext.c	2004-08-16 04:29:50 -0400
+++ loader/ext.c	2004-08-22 23:54:17 -0400
@@ -459,6 +459,7 @@ LPVOID WINAPI VirtualAlloc(LPVOID addres
     void* answer;
     int fd;
     long pgsz;
+    int flags;
 
     printf("VirtualAlloc(0x%08X, %u, 0x%08X, 0x%08X)\n", (unsigned)address, size, type, protection);
 
@@ -518,8 +519,13 @@ LPVOID WINAPI VirtualAlloc(LPVOID addres
 	}
     }
 
+    flags = MAP_PRIVATE;
+#ifdef MAP_NOUNMAP
+    if (address)
+	flags |= MAP_FIXED|MAP_NOUNMAP;
+#endif
     answer=mmap(address, size, PROT_READ | PROT_WRITE | PROT_EXEC,
-		MAP_PRIVATE, fd, 0);
+		flags, fd, 0);
 //    answer=FILE_dommap(-1, address, 0, size, 0, 0,
 //	PROT_READ | PROT_WRITE | PROT_EXEC, MAP_PRIVATE);
     close(fd);
========================================================================

This could be done in mplayer right now since, to my knowledge, no
operating system defines MAP_NOUNMAP at this point in time.

The advantage in performance would be that no unusable mapping/unmapping
would be done (all those pages allocated in mmap() to be immediately
followed by their unallocation in munmap()).  The proposed Linux
kernel patch represents an early return from its mmap() implementation.
Since there would be no unnecessary munmap() in this case, that's one
system call (and its consequent context switches) saved.



The behavior of MAP_FIXED without MAP_NOUNMAP is still very useful in
other circumstances (but not this one) and it would not be a good idea
to lobby for a change in the behavior of MAP_FIXED specified alone,
especially since its current behavior is mandated by POSIX.



(Sorry for not putting the patches in attachments as per mailing list
policy, but they are short and I mean for them to be quoted and argued...)