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

[Yury Gribov]

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