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

[Pavel Kopyl]

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