From owner-freebsd-hackers@FreeBSD.ORG Fri Aug 24 17:25:52 2012 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 1941F106564A; Fri, 24 Aug 2012 17:25:52 +0000 (UTC) (envelope-from jhein@symmetricom.com) Received: from duck.symmetricom.us (duck.symmetricom.us [206.168.13.214]) by mx1.freebsd.org (Postfix) with ESMTP id 885408FC16; Fri, 24 Aug 2012 17:25:46 +0000 (UTC) Received: from gromit.timing.com (gromit.timing.com [206.168.13.209]) by duck.symmetricom.us (8.14.5/8.14.5) with ESMTP id q7OHPiAk014470; Fri, 24 Aug 2012 11:25:44 -0600 (MDT) (envelope-from jhein@symmetricom.com) Received: from gromit.timing.com (localhost [127.0.0.1]) by gromit.timing.com (8.14.5/8.14.5) with ESMTP id q7OHP7aJ045088; Fri, 24 Aug 2012 11:25:07 -0600 (MDT) (envelope-from jhein@gromit.timing.com) Received: (from jhein@localhost) by gromit.timing.com (8.14.5/8.14.5/Submit) id q7OHP7GG045087; Fri, 24 Aug 2012 11:25:07 -0600 (MDT) (envelope-from jhein) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <20535.47346.913434.799005@gromit.timing.com> Date: Fri, 24 Aug 2012 11:25:06 -0600 From: John Hein To: Konstantin Belousov In-Reply-To: <20120824162338.GV33100@deviant.kiev.zoral.com.ua> References: <20535.39682.330250.337503@gromit.timing.com> <20120824162338.GV33100@deviant.kiev.zoral.com.ua> X-Mailer: VM 8.2.0b-trunk-1408 under 23.4.1 (i386-portbld-freebsd7.4) Cc: hackers@freebsd.org, secteam@freebsd.org Subject: Re: LD_PRELOADed code before or after exec - different behavior after 6.x X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2012 17:25:52 -0000 Answers inline... Konstantin Belousov wrote at 19:23 +0300 on Aug 24, 2012: > On Fri, Aug 24, 2012 at 09:17:22AM -0600, John Hein wrote: > > > > head sl.cc pe.c > > ==> sl.cc <== > > #include > > #include > > class C > > { > > public: > > C(){ > > printf("C\n"); > > unsetenv("XXX"); > > } > > }; > > static C c; > > > > ==> pe.c <== > > #include > > #include > > int > > main() > > { > > char *p=getenv("XXX"); > > if (p != NULL) > > printf("XXX=%s\n",p); > > return 0; > > } > > > > > > % g++ -fpic -shared sl.cc -o sl.so > > % gcc pe.c -o pe > > > > 7.x & 8.x ... > > % sh -c 'XXX=1 LD_PRELOAD=$(pwd)/sl.so pe' > > C > > XXX=1 > > > > 6.x & 4.x ... > > % sh -c 'XXX=1 LD_PRELOAD=$(pwd)/sl.so pe' > > C > > > > > > In 6.x and earlier (fedora 16, too), the unsetenv clears the XXX env > > var apparently in time to affect the exec'd process. In 8.x & 9.x, it > > seems the environment is set and passed to the exec'd process and the > > LD_PRELOADed code does not affect that despite its best efforts. > > > > It seems to me that 6.x behavior is more correct, but I'm seeking > > opinions before contemplating if / how to put together a fix. > > > I suppose that this is a fallback from the POSIXification of putenv(). > The libc image of the environment block is now referenced through > the environ var. Since csu always reinitialize the environ, effect > of the changes made by preloaded dso is cleared. > > At the end of the message is the patch for HEAD which seems to fix the > issue. It is not applicable to stable/9 or 8. You could try to change > lib/csu//crt1.c to replace unconditional > environ = env; > assignment with > if (environ == NULL) > environ = env; > . Thanks. This fixes my reported problem - tested on 8.x. > Unfortunately, because csu is linked to the binary, you have to relink > the binary after applying the patch and rebuilding the world. In other > words, old binaries cannot be fixed. > > Committing this requires secteam@ review, AFAIR. Indeed this could be a sec issue if someone relies on preloaded code clearing something in the environment. secteam CC'd > diff --git a/lib/csu/amd64/crt1.c b/lib/csu/amd64/crt1.c > index f33aad6..3740e73 100644 > --- a/lib/csu/amd64/crt1.c > +++ b/lib/csu/amd64/crt1.c > @@ -61,9 +61,7 @@ _start(char **ap, void (*cleanup)(void)) > argc = *(long *)(void *)ap; > argv = ap + 1; > env = ap + 2 + argc; > - environ = env; > - if (argc > 0 && argv[0] != NULL) > - handle_progname(argv[0]); > + handle_argv(argc, argv, env); > > if (&_DYNAMIC != NULL) > atexit(cleanup); > diff --git a/lib/csu/arm/crt1.c b/lib/csu/arm/crt1.c > index 127c28d..e3529b8 100644 > --- a/lib/csu/arm/crt1.c > +++ b/lib/csu/arm/crt1.c > @@ -98,10 +98,7 @@ __start(int argc, char **argv, char **env, struct ps_strings *ps_strings, > const struct Struct_Obj_Entry *obj __unused, void (*cleanup)(void)) > { > > - environ = env; > - > - if (argc > 0 && argv[0] != NULL) > - handle_progname(argv[0]); > + handle_argv(argc, argv, env); > > if (ps_strings != (struct ps_strings *)0) > __ps_strings = ps_strings; > diff --git a/lib/csu/common/ignore_init.c b/lib/csu/common/ignore_init.c > index e3d2441..89b3734 100644 > --- a/lib/csu/common/ignore_init.c > +++ b/lib/csu/common/ignore_init.c > @@ -87,14 +87,18 @@ handle_static_init(int argc, char **argv, char **env) > } > > static inline void > -handle_progname(const char *v) > +handle_argv(int argc, char *argv[], char **env) > { > const char *s; > > - __progname = v; > - for (s = __progname; *s != '\0'; s++) { > - if (*s == '/') > - __progname = s + 1; > + if (environ == NULL) > + environ = env; > + if (argc > 0 && argv[0] != NULL) { > + __progname = argv[0]; > + for (s = __progname; *s != '\0'; s++) { > + if (*s == '/') > + __progname = s + 1; > + } > } > } > > diff --git a/lib/csu/i386-elf/crt1_c.c b/lib/csu/i386-elf/crt1_c.c > index 3249069..65de04c 100644 > --- a/lib/csu/i386-elf/crt1_c.c > +++ b/lib/csu/i386-elf/crt1_c.c > @@ -61,10 +61,7 @@ _start1(fptr cleanup, int argc, char *argv[]) > char **env; > > env = argv + argc + 1; > - environ = env; > - if (argc > 0 && argv[0] != NULL) > - handle_progname(argv[0]); > - > + handle_argv(argc, argv, env); > if (&_DYNAMIC != NULL) > atexit(cleanup); > else > diff --git a/lib/csu/mips/crt1.c b/lib/csu/mips/crt1.c > index 1968f06..95348b7 100644 > --- a/lib/csu/mips/crt1.c > +++ b/lib/csu/mips/crt1.c > @@ -71,9 +71,7 @@ __start(char **ap, > argc = * (long *) ap; > argv = ap + 1; > env = ap + 2 + argc; > - environ = env; > - if (argc > 0 && argv[0] != NULL) > - handle_progname(argv[0]); > + handle_argv(argc, argv, env); > > if (&_DYNAMIC != NULL) > atexit(cleanup); > diff --git a/lib/csu/powerpc/crt1.c b/lib/csu/powerpc/crt1.c > index c3be90d..d1a3ea0 100644 > --- a/lib/csu/powerpc/crt1.c > +++ b/lib/csu/powerpc/crt1.c > @@ -81,10 +81,8 @@ _start(int argc, char **argv, char **env, > struct ps_strings *ps_strings) > { > > - environ = env; > > - if (argc > 0 && argv[0] != NULL) > - handle_progname(argv[0]); > + handle_argv(argc, argv, env); > > if (ps_strings != (struct ps_strings *)0) > __ps_strings = ps_strings; > diff --git a/lib/csu/powerpc64/crt1.c b/lib/csu/powerpc64/crt1.c > index a7c3581..35c5a6e 100644 > --- a/lib/csu/powerpc64/crt1.c > +++ b/lib/csu/powerpc64/crt1.c > @@ -81,10 +81,7 @@ _start(int argc, char **argv, char **env, > struct ps_strings *ps_strings) > { > > - environ = env; > - > - if (argc > 0 && argv[0] != NULL) > - handle_progname(argv[0]); > + handle_argv(argc, argv, env); > > if (ps_strings != (struct ps_strings *)0) > __ps_strings = ps_strings; > diff --git a/lib/csu/sparc64/crt1.c b/lib/csu/sparc64/crt1.c > index 3b3ecc2..e11ae39 100644 > --- a/lib/csu/sparc64/crt1.c > +++ b/lib/csu/sparc64/crt1.c > @@ -85,9 +85,7 @@ _start(char **ap, void (*cleanup)(void), struct Struct_Obj_Entry *obj __unused, > argc = *(long *)(void *)ap; > argv = ap + 1; > env = ap + 2 + argc; > - environ = env; > - if (argc > 0 && argv[0] != NULL) > - handle_progname(argv[0]); > + handle_argv(argc, argv, env); > > if (&_DYNAMIC != NULL) > atexit(cleanup); > [DELETED ATTACHMENT , application/pgp-signature]