This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.



On 05/27/2015 04:45 PM, H.J. Lu wrote:

On Wed, May 27, 2015 at 6:36 AM, Yury Gribov <y.gribov@samsung.com> wrote:

On 05/27/2015 04:23 PM, H.J. Lu wrote:

On Wed, May 27, 2015 at 6:12 AM, Yury Gribov <y.gribov@samsung.com> wrote:

On 05/27/2015 04:04 PM, H.J. Lu wrote:


On Wed, May 27, 2015 at 5:17 AM, Pavel Kopyl <p.kopyl@samsung.com>
wrote:




On 05/13/2015 02:40 PM, Pavel Kopyl wrote:





On 04/24/2015 02:47 PM, H.J. Lu wrote:



On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com>
wrote:




On 03/03/2015 12:13 PM, Pavel Kopyl wrote:




On 03/01/2015 10:17 PM, Mike Frysinger wrote:



On 27 Feb 2015 16:32, Pavel Kopyl wrote:



--- /dev/null
+++ b/elf/tst-unique5lib.cc
@@ -0,0 +1,13 @@
+



i know existing tests are bad examples, but lets try and start
fixing
that.
namely, there should be a header here giving a quick overview of
what
it
is
exactly you're testing for, and a BZ reference.


+extern int not_exist ();
+
+inline int make_unique ()
+{
+  static int unique;
+  return ++unique;
+}
+
+int foo ()
+{
+  return make_unique () + not_exist ();
+}



i don't know if this is just copy & pasting, but prototypes that
do
not
intend
to take args should always be (void).
-mike




Thanks for review, I fixed that in patch v3.



Ping.


Some comments:

1. The bug report is against STB_GNU_UNIQUE.  But I don't see
STB_GNU_UNIQUE in
testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
2. Your testcase ignores dlopen error.  Why should it work at all?
3. Your testcase doesn't use test-skeleton.c.


Thanks for review!

1. I added some comments in testcase to be more clear.

2. Actually the call below is expected to return NULL because of
undefined
symbol in the library.

dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);

It's a goal of this testcase: I need some library with STB_GNU_UNIQUE
symbols that returns error when trying to load it.
In new patch version I especially point out to this fact by checking
return value.

3. It seems for me that template of the test-skeleton.c is not
appropriate
because this testcase has two related binary - library and executable.


-Pavel



Ping.


Since you changed DF_1_NODELETE, how did you test it?  Does your test
fail without the DF_1_NODELETE change?



AFAIU DF_1_NODELETE change is necessary. Otherwise unique symbols force
library to remain loaded in half-initialized state (with missing
dependencies). This usecase is demonstrated by the testcase in patch.


But unique symbols != DF_1_NODELETE:


DF_1_NODELETE is set in do_lookup_unique at runtime (Paul, am I right?):

       enter_unique_sym (entries, size,
                         new_hash, strtab + sym->st_name, sym, map);

       if (map->l_type == lt_loaded)
         /* Make sure we don't unload this object by
            setting the appropriate flag.  */
         ((struct link_map *) map)->l_flags_1 |= DF_1_NODELETE;

-Y

Should we set DF_1_NODELETE when dlopen fails?  What happens
when dlopen a  DF_1_NODELETE fails?  Do we keep it in memory
even when dopen fails?
As I know there are following ways how to set DF_1_NODELETE flag for a library. 

1. Unique symbols.
2, Load with RTLD_NODELETE flag.
3. Link with "-z nodelete" option.
As for RTLD_NODELETE, there is no problem: dlopen exits in case of error before DF_1_NODELETE is set. If dlopen fails both in first and third cases this leads to locking libraries in half-initialized state. I think this is a bug. 
Suggested patch fixes both usecases.

-Pavel
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]