Re: [PATCH][BZ #15022] Correct global-scope dlopen issues in static executables

[Roland McGrath <roland at hack dot frob dot com>]

>  I have therefore created one bug report only, #15022.

That's fine.

>  Please elaborate on what user-visible semantics you would like 
> specifically to preserve.

I can no longer tell what I had in mind.  I must have misinterpreted the
description in your original posting.

>  Finally, if you'd rather I split the getpagesize tests off and submitted 
> them separately, then I'm fine with that; obviously the tests would have 
> to be added after the bug fix itself or otherwise they would regress right 
> away.

That would be ideal in the abstract.  But if you find it a hassle, I don't
really object to rolling them in.

> @@ -177,6 +178,26 @@ LIBC_START_MAIN (int (*main) (int, char 
>       we need to setup errno.  */
>    __pthread_initialize_minimal ();
>  
> +  /* Create a dummy link_map for the executable, used by dlopen to
> +     access the global scope.  We don't export any symbols ourselves,
> +     so this can be minimal.  */
> +  struct link_map **new_global;
> +  struct link_map *main_map;

Don't pre-declare, just use initialized declarations.

> +  main_map = _dl_new_object ("", "", lt_executable, NULL, 0, LM_ID_BASE);
> +  assert (main_map != NULL);
> +
> +  _dl_add_to_namespace_list (main_map, LM_ID_BASE);
> +  assert (main_map == GL(dl_ns)[LM_ID_BASE]._ns_loaded);
> +  GL(dl_nns) = 1;
> +
> +  new_global = malloc (sizeof (struct link_map **));
> +  assert (new_global != NULL);
> +  *new_global = main_map;
> +  main_map->l_searchlist.r_list = new_global;
> +  main_map->l_searchlist.r_nlist = 1;
> +  GL(dl_ns)[LM_ID_BASE]._ns_main_searchlist = &main_map->l_searchlist;

This stuff seems like it belongs in _dl_non_dynamic_init rather than here.
Is there a problem with putting it there?

It would be even better if as much of this as possible could be done with
static initialization rather than code that runs at startup.  Can you try
to do it that way (in elf/dl-support.c) and see if it's not desperately
difficult?

Since dl_nns will never be zero now, there is some dead code in _dl_open
#ifndef SHARED.

>  tststatic-ENV = LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)elf
>  tststatic2-ENV = LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)elf
> +tststatic3-ENV = LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)elf
> +tststatic4-ENV = LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)elf
> +tststatic5-ENV = LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)elf
> +tststatic6-ENV = LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)elf

How about we make all the repetitions just use $(tststatic-ENV) instead?

> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ glibc-fsf-trunk-quilt/dlfcn/modstatic3.c	2013-01-24 12:24:09.087820478 +0000
> @@ -0,0 +1,30 @@
> +/* Copyright (C) 2013 Free Software Foundation, Inc.

Top line of every new file is a descriptive comment.

Most of the test code could use some more comments explaining what it's
testing and why.

> +  setfoo (0x5500ffaa);
> +
> +  foo = *foop;
> +  if (foo != 0x5500ffaa)

Use macros instead of repeating the same magic number.


I don't understand the situation with _dl_static_init.  It seems wrong that
DL_STATIC_INIT should ever be called more than once on the same module.
It's just like initializers.  Why isn't it done inside _dl_init or where
_dl_init is called in dl_open_worker?  Then it would never be called at all
for the dummy "main executable" link_map.  I think we should get that
cleaned up before attempting your fix.


Thanks,
Roland