Detecting dlclose() on an already closed handle?

Detecting dlclose() on an already closed handle?

[Carlos O'Donell]

In elf/dl-close.c we have this code:

  if (__builtin_expect (map->l_direct_opencount, 1) == 0)
    _dl_signal_error (0, map->l_name, NULL, N_("shared object not open"));

This code only detects double dlclose() when the dlclose() is recursively
called by the same thread's own destructors otherwise it accesses potentially
unmapped memory (or memory allocated to something else) and results in
undefined behaviour (SIGSEGV, corrupting the new memory if it looks enough
like a struct link_map to allow the dlclose to continue).

We cannot catch  a double-dlclose reliably because the struct link_map
used to test for this might have been freed already.

The first dlclose drops the l_direct_opencount to 0, and the object _may_
be unmapped and the struct link_map freed.

The only way to reliably test for an error would be, AFAICT, to store the
information in the 'void*' whose storage is owned by the caller. You'd have
to use an ID and then never recycle the IDs. For 32-bit the ABA happens
pretty quickly if you're doing lots of dlopen/dlclose, and we could easily
use a larger counter internally to allow the ABA, at the cost of loosing
the ability to detect double dlclose() for a dlclose() that happened
4billion dlopen() calls ago.

POSIX says:
~~~
If the referenced symbol table handle was successfully closed, dlclose() 
shall return 0. If handle does not refer to an open symbol table handle 
or if the symbol table handle could not be closed, dlclose() shall 
return a non-zero value. More detailed diagnostic information shall 
be available through dlerror().
~~~

It appears that dlclose() should be able to detect the handle is not
open or invalid. We only support that reliably in one scenario:
where you recursively call dlclose() and we detect it. All other
detections, like an outright double dlclose(), result in reading
memory that was already freed e.g.

Valgrind notes the access of map->l_flags_1 to check for DF_1_NODELETE
from memory already freed on the second dlclose():

==8526== Invalid read of size 1
==8526==    at 0x4014D61: _dl_close (dl-close.c:820)
==8526==    by 0x5157C60: _dl_catch_error (dl-error-skeleton.c:198)
==8526==    by 0x53D8598: _dlerror_run (dlerror.c:163)
==8526==    by 0x53D805E: dlclose (dlclose.c:46)
==8526==    by 0x400F709: _dl_fini (dl-fini.c:235)
==8526==    by 0x506EA6F: __run_exit_handlers (exit.c:83)
==8526==    by 0x506EAC9: exit (exit.c:105)
==8526==    by 0x50594C7: (below main) (libc-start.c:320)

Arguably glibc trades off complexity for what is effectively a user
bug that must be detected by heap consistency checkers. It rarely
leads to a SIGSEGV because of malloc's chunk cache.

However, POSIX appears to allow it, and using ID-based handles is
an attractive solution. It's also not always easy to determine if
you have already called dlclose() on a handle without additional
synchronization, and making that fail-safe certainly helps application
developers. The problem is that with ABA on 32-bit the second dlclose()
might close a different library, a problem which already exists today
since malloc may return the same address for a struct link_map and the
second dlclose() may close that new library, so it's no worse.

Any thoughts about how to handle this reliably?

-- 
Cheers,
Carlos.

Re: Detecting dlclose() on an already closed handle?

[Szabolcs Nagy]

* Carlos O'Donell <carlos@redhat.com> [2016-12-22 11:56:45 -0500]:
> The only way to reliably test for an error would be, AFAICT, to store the
> information in the 'void*' whose storage is owned by the caller. You'd have
> to use an ID and then never recycle the IDs. For 32-bit the ABA happens
> pretty quickly if you're doing lots of dlopen/dlclose, and we could easily
> use a larger counter internally to allow the ABA, at the cost of loosing
> the ability to detect double dlclose() for a dlclose() that happened
> 4billion dlopen() calls ago.
> 
> POSIX says:
> ~~~
> If the referenced symbol table handle was successfully closed, dlclose() 
> shall return 0. If handle does not refer to an open symbol table handle 
> or if the symbol table handle could not be closed, dlclose() shall 
> return a non-zero value. More detailed diagnostic information shall 
> be available through dlerror().
> ~~~
...
> It appears that dlclose() should be able to detect the handle is not
> open or invalid. We only support that reliably in one scenario:
> where you recursively call dlclose() and we detect it. All other

if invalid handle detection is not reliable (because the handle
may be reused) then i think it can only be a hardening or debug
measure, but should not be part of the interface contract: it
must be the responsibility of the caller to use a valid handle.
(unloading a random module in case of double dlclose is not a
useful operation.)

so this seems to be a posix bug.

Re: Detecting dlclose() on an already closed handle?

[Carlos O'Donell]

On 12/22/2016 03:50 PM, Szabolcs Nagy wrote:
> * Carlos O'Donell <carlos@redhat.com> [2016-12-22 11:56:45 -0500]:
>> The only way to reliably test for an error would be, AFAICT, to store the
>> information in the 'void*' whose storage is owned by the caller. You'd have
>> to use an ID and then never recycle the IDs. For 32-bit the ABA happens
>> pretty quickly if you're doing lots of dlopen/dlclose, and we could easily
>> use a larger counter internally to allow the ABA, at the cost of loosing
>> the ability to detect double dlclose() for a dlclose() that happened
>> 4billion dlopen() calls ago.
>>
>> POSIX says:
>> ~~~
>> If the referenced symbol table handle was successfully closed, dlclose() 
>> shall return 0. If handle does not refer to an open symbol table handle 
>> or if the symbol table handle could not be closed, dlclose() shall 
>> return a non-zero value. More detailed diagnostic information shall 
>> be available through dlerror().
>> ~~~
> ...
>> It appears that dlclose() should be able to detect the handle is not
>> open or invalid. We only support that reliably in one scenario:
>> where you recursively call dlclose() and we detect it. All other
> 
> if invalid handle detection is not reliable (because the handle
> may be reused) then i think it can only be a hardening or debug
> measure, but should not be part of the interface contract: it
> must be the responsibility of the caller to use a valid handle.
> (unloading a random module in case of double dlclose is not a
> useful operation.)

That is a very good point, particularly about the reliability
and its relationship to hardening/debug.

> so this seems to be a posix bug.

I agree. The requirement means some state of the dlopen handle
must be kept forever and that has infinite memory requirements.

Bug filed:
http://austingroupbugs.net/view.php?id=1110

-- 
Cheers,
Carlos.

Re: Detecting dlclose() on an already closed handle?

[Carlos O'Donell]

On 12/22/2016 11:56 AM, Carlos O'Donell wrote:
> POSIX says:
> ~~~
> If the referenced symbol table handle was successfully closed, dlclose() 
> shall return 0. If handle does not refer to an open symbol table handle 
> or if the symbol table handle could not be closed, dlclose() shall 
> return a non-zero value. More detailed diagnostic information shall 
> be available through dlerror().
> ~~~

I read too much into this.

The only thing the text guarantees is that the handle currently points to
an open symbol table (loaded library) and we don't care if it originally
referenced a different library due to handle reuse.

Therefore the only solution being forced is that the implementation must
maintain a list of open objects. We already do that for dlopen/dlclose.
Where it's costly is dlsym where you have to do handle validation.

Therefore I think we can fix this without much trouble.

-- 
Cheers,
Carlos.

Re: Detecting dlclose() on an already closed handle?

[Szabolcs Nagy]

* Carlos O'Donell <carlos@redhat.com> [2016-12-25 22:10:13 -0500]:
> On 12/22/2016 11:56 AM, Carlos O'Donell wrote:
> > POSIX says:
> > ~~~
> > If the referenced symbol table handle was successfully closed, dlclose() 
> > shall return 0. If handle does not refer to an open symbol table handle 
> > or if the symbol table handle could not be closed, dlclose() shall 
> > return a non-zero value. More detailed diagnostic information shall 
> > be available through dlerror().
> > ~~~
> 
> I read too much into this.
> 
> The only thing the text guarantees is that the handle currently points to
> an open symbol table (loaded library) and we don't care if it originally
> referenced a different library due to handle reuse.
> 
> Therefore the only solution being forced is that the implementation must
> maintain a list of open objects. We already do that for dlopen/dlclose.
> Where it's costly is dlsym where you have to do handle validation.
> 
> Therefore I think we can fix this without much trouble.

yes it can be fixed, my point was that the return value is not
something the caller can rely on in a multi-threaded process.

i still think it's a posix bug to require extra work when the
result is not useful. (even libc calls may dlopen, as they do
in glibc, so the caller cannot reasonably avoid handle reuse
in case of double dlclose).

Re: Detecting dlclose() on an already closed handle?

[Adhemerval Zanella]

On 26/04/2017 19:48, Rm Beer wrote:
> The problem of double dlclose() not is a bug. I read about of this in
> the listing and view that dlclose() seem has already everything the
> need for clean from the memory: check the recursive dlclose(), free
> memory, etc.
> 
> The problem is from the programmer, you known that where you
> close/free anything, in the memory have a garbage of the last data
> used. This garbage if not clean or replaced by any other new memory,
> have misleading datas.
> The programmer must check if already have execute a double dlclose().
> No POSIX, glibc or other libraries, only the programmer.
> 
> Can are confuse where saying all dlopen() have all dlclose(). Not
> confused this with double dlclose() for dlopen(). Where you use
> dlclose() for a dlopen(), the pair is complete. The (void*) of handler
> is now a garbage that must be forgotten.
> 
> Not is a bug.
> 

You must be referring to BZ#20990 and BZ#14989, which were the one we
talked about on IRC (BZ#20990 was recently closed since it was a
duplicate of BZ#14989).

The issue here is detecting dlclose on an invalid handler is a POSIX
requirement in current standard [1]:

"If handle does not refer to an open symbol table handle or if the symbol 
 table handle could not be closed"

It was raised two defects/clarification on Austin groups [2] [3].  The
first one, 639, asked to make the dlclose on invalid handler an undefined
behavior and it was reject by POSIX saying some application requires
this behaviour.  The second one, 1110, asked for slight change in returned
code both the faulty behaviour (dlclose must return a non-zero value
to may return a non-zero value) and Carlos in his last comment to close
this issue in the end.

In a short, with current decisions and clarification from Austin Group,
GLIBC implementation is non-conforming regarding POSIX.  The BZ#14989
is valid and we should aims to fix it.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/ 
[2] http://austingroupbugs.net/view.php?id=639
[3] http://austingroupbugs.net/view.php?id=1110

RE: Detecting dlclose() on an already closed handle?

[Rm Beer]

The problem of double dlclose() not is a bug. I read about of this in
the listing and view that dlclose() seem has already everything the
need for clean from the memory: check the recursive dlclose(), free
memory, etc.

The problem is from the programmer, you known that where you
close/free anything, in the memory have a garbage of the last data
used. This garbage if not clean or replaced by any other new memory,
have misleading datas.
The programmer must check if already have execute a double dlclose().
No POSIX, glibc or other libraries, only the programmer.

Can are confuse where saying all dlopen() have all dlclose(). Not
confused this with double dlclose() for dlopen(). Where you use
dlclose() for a dlopen(), the pair is complete. The (void*) of handler
is now a garbage that must be forgotten.

Not is a bug.