Date: Fri, 24 Aug 2012 11:25:06 -0600 From: John Hein <jhein@symmetricom.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: hackers@freebsd.org, secteam@freebsd.org Subject: Re: LD_PRELOADed code before or after exec - different behavior after 6.x Message-ID: <20535.47346.913434.799005@gromit.timing.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <cstdio>
> > #include <cstdlib>
> > class C
> > {
> > public:
> > C(){
> > printf("C\n");
> > unsetenv("XXX");
> > }
> > };
> > static C c;
> >
> > ==> pe.c <==
> > #include <stdio.h>
> > #include <stdlib.h>
> > 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/<arch>/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 <no suggested filename>, application/pgp-signature]
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20535.47346.913434.799005>