i386/127387: Inline assembler in x86 _start() in crt1.c only works by chance

[Christoph Mallon]

>Number:         127387
>Category:       i386
>Synopsis:       Inline assembler in x86 _start() in crt1.c only works by chance
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-i386
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sun Sep 14 20:00:10 UTC 2008
>Closed-Date:
>Last-Modified:
>Originator:     Christoph Mallon
>Release:        n/a
>Organization:
>Environment:
n/a
>Description:
Disassembling _start() of a common binary shows this[0]:
08048450 <_start>:
 8048450:       55                      push   %ebp
 8048451:       89 e5                   mov    %esp,%ebp
 8048453:       57                      push   %edi
 8048454:       56                      push   %esi
 8048455:       53                      push   %ebx
 8048456:       83 ec 0c                sub    $0xc,%esp
 8048459:       83 e4 f0                and    $0xfffffff0,%esp
 804845c:       8b 5d 04                mov    0x4(%ebp),%ebx
 804845f:       89 d7                   mov    %edx,%edi
[...]

The last instruction shown is generated via inline assembler in src/lib/csu/i386-elf/crt1.c.  It copies the rtld "cleanup" function pointer from %edx into a different register[1].  Just before the only declared parameter ist loaded from stack into %ebx.  In the source file this instruction is below the inline assembler to load "cleanup", but of course the compiler is free to shuffle things around.  Now the problematic part:  The compiler chose %ebx as destination register.  It could have chosen %edx, because nobody told him that there is a precious value in there.  In fact there is no safe way to ensure that %edx is not destroyed before it is read in this function.  I present a replacement, which does the ugly handling[2] in a small global asm block.


[0] objdump -d $SOME_UNSTRIPPED_BINARY | $PAGER or just objdump -d /usr/lib/crt1.o | $PAGER
[1] The used inline assembler statement is unnecessarily complicated, but that's another story.  Simpler is asm("" : "=d" (cleanup));.
[2] Other parts like fetching argc from the stack and aligning the stack via inline assembler are far beyond legal C, too.
>How-To-Repeat:

>Fix:
See attachment.

Patch attached with submission follows:

Index: crt1.c
===================================================================
--- crt1.c	(Revision 183012)
+++ crt1.c	(Arbeitskopie)
@@ -38,8 +38,6 @@
 extern int _DYNAMIC;
 #pragma weak _DYNAMIC
 
-typedef void (*fptr)(void);
-
 extern void _fini(void);
 extern void _init(void);
 extern int main(int, char **, char **);
@@ -55,35 +53,16 @@
 char **environ;
 const char *__progname = "";
 
-static __inline fptr
-get_rtld_cleanup(void)
-{
-	fptr retval;
-
-#ifdef	__GNUC__
-	__asm__("movl %%edx,%0" : "=rm"(retval));
-#else
-	retval = (fptr)0; /* XXXX Fix this for other compilers */
-#endif
-	return(retval);
-}
-
 /* The entry function. */
-void
-_start(char *ap, ...)
+static void
+#ifdef __GNUC__
+__attribute__((regparm(3), noreturn, used))
+#endif
+internal_start(int argc, void (*cleanup)(void), char *argv[])
 {
-	fptr cleanup;
-	int argc;
-	char **argv;
 	char **env;
 	const char *s;
 
-#ifdef __GNUC__
-	__asm__("and $0xfffffff0,%esp");
-#endif
-	cleanup = get_rtld_cleanup();
-	argv = &ap;
-	argc = *(long *)(void *)(argv - 1);
 	env = argv + argc + 1;
 	environ = env;
 	if (argc > 0 && argv[0] != NULL) {
@@ -110,4 +89,22 @@
 	exit( main(argc, argv, env) );
 }
 
-__asm__(".ident\t\"$FreeBSD$\"");
+#ifdef __GNUC__
+__asm__(
+	".text                  \n\t"
+	".p2align 4,,15         \n"
+	".globl _start          \n\t"
+	".type _start, @function\n"
+	"_start:                \n\t"
+	"pushl %ebp             \n\t"
+	"movl  %esp,        %ebp\n\t"
+	"andl  $0xFFFFFFF0, %esp\n\t" // align stack
+	"movl  4(%ebp),     %eax\n\t" // argc
+	/* edx already contains rtld cleanup */
+	"leal  8(%ebp),     %ecx\n\t" // argv
+	"call  internal_start   \n\t"
+	".size _start, . - _start"
+);
+#endif
+
+__asm__(".ident\t\"$FreeBSD: src/lib/csu/i386-elf/crt1.c,v 1.15 2005/10/07 22:13:17 bde Exp $\"");


>Release-Note:
>Audit-Trail:
>Unformatted: