[PATCHv5,PING^2,BZ,#17833] _dl_close_worker() does not release inconsistent objects.

  On 06/05/2015 09:44 AM, Yury Gribov wrote:
> On 05/29/2015 08:38 PM, Pavel Kopyl wrote:
>> On 05/29/2015 12:31 AM, Pavel Kopyl wrote:
>>> On 05/28/2015 01:06 AM, H.J. Lu wrote:
>>>> On Wed, May 27, 2015 at 2:38 PM, Pavel Kopyl <p.kopyl@samsung.com>
>>>> wrote:
>>>>>>>>> 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.
>>>> Assume there is a problem with DF_1_NODELETE, which can be
>>>> set explicitly as well as implicitly.  Your testcase only covers
>>>> implicitly DF_1_NODELETE, not explicitly  DF_1_NODELETE.
>>>> I think we should verify the status of explicitly DF_1_NODELETE.
>>>>
>>>> If explicitly  DF_1_NODELETE works fine, then we only have a
>>>> problem with implicitly DF_1_NODELETE.
>>>>
>>>> If explicitly  DF_1_NODELETE doesn't work, we need to fix it
>>>> first and implicitly DF_1_NODELETE may just work.
>>>
>>> Ok, I prepared the following testcase.
>>>
>>> tst-nodelete.cc:
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <dlfcn.h>
>>>
>>> int
>>> main (void)
>>> {
>>>   int ret = 0;
>>>
>>>   /* This is a test for correct handling of dlopen failures for
>>> library that
>>>      is loaded with RTLD_NODELETE flag. The first dlopen should fail
>>> because
>>>      of undefined symbols in shared library. The second dlopen then
>>> verifies
>>>      that library was properly unloaded.  */
>>>   if (dlopen ("./tst-nodeletelib.so", RTLD_NOW | RTLD_NODELETE) != NULL
>>>       || dlopen ("./tst-nodeletelib.so", RTLD_LAZY | RTLD_NOLOAD) !=
>>> NULL)
>>>     {
>>>       printf("RTLD_NOLOAD test failed\n");
>>>       ret = 1;
>>>     }
>>>
>>>   /* This is a test for correct handling of dlopen failures for
>>> library that
>>>      is linked with '-z nodelete' option and hence has DF_1_NODELETE
>>> flag.
>>>      The first dlopen should fail because of undefined symbols in shared
>>>      library. The second dlopen then verifies that library was properly
>>>      unloaded.  */
>>>   if (dlopen ("./tst-znodeletelib.so", RTLD_NOW) != NULL
>>>       || dlopen ("./tst-znodeletelib.so", RTLD_LAZY | RTLD_NOLOAD) !=
>>> NULL)
>>>     {
>>>       printf("-z \'nodelete\' test failed\n");
>>>       ret = 1;
>>>     }
>>>
>>>    /* This is a test for correct handling of dlopen failures for library
>>>      with unique symbols. The first dlopen should fail because of
>>> undefined
>>>      symbols in shared library. The second dlopen then verifies that
>>> library
>>>      was properly unloaded.  */
>>>   if (dlopen ("./tst-unique5lib.so", RTLD_NOW) != NULL
>>>       || dlopen ("./tst-unique5lib.so", RTLD_LAZY | RTLD_NOLOAD) !=
>>> NULL)
>>>     {
>>>       printf("Unique symbols test failed\n");
>>>       ret = 1;
>>>     }
>>>
>>>   if (!ret)
>>>     printf ("SUCCESS\n");
>>>
>>>   return ret;
>>> }
>>>
>>>
>>> tst-unique5lib.cc:
>>> extern int not_exist (void);
>>>
>>> inline int make_unique (void)
>>> {
>>> /* Static variables in inline functions and classes
>>>    generate STB_GNU_UNIQUE symbols.  */
>>>   static int unique;
>>>   return ++unique;
>>> }
>>>
>>> int foo (void)
>>> {
>>>   return make_unique () + not_exist ();
>>> }
>>>
>>>
>>> tst-nodeletelib.cc:
>>> extern int not_exist (void);
>>>
>>> int foo (void)
>>> {
>>>   return  not_exist ();
>>> }
>>>
>>>
>>> g++ -o tst-nodelete tst-nodelete.cc -ldl
>>> g++ -shared -fPIC -o tst-nodeletelib.so tst-nodeletelib.cc
>>> g++ -shared -fPIC -Wl,-z,nodelete -o tst-znodeletelib.so
>>> tst-nodeletelib.cc
>>> ./tst-nodelete
>>> -z 'nodelete' test failed
>>> Unique symbols test failed
>>>
>>>
>>> Thus, besides unique symbols we have a problem with explicitly
>>> DF_1_NODELETE when library is built with "-Wl,-z,nodelete".
>>> I checked that patch fixes this usecase too. Now I'm going to update
>>> testcase in make check.
>>
>> This is new patch with updated testcase.
>>
>> -Pavel
>
> Ping.

[PATCHv5,PING^2,BZ,#17833] _dl_close_worker() does not release inconsistent objects.

Commit Message

Patch