Bug 14989 - Implement double dlclose() detection as required by POSIX
Summary: Implement double dlclose() detection as required by POSIX
Status: REOPENED
Alias: None
Product: glibc
Classification: Unclassified
Component: dynamic-link (show other bugs)
Version: 2.16
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 20990 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-12-30 08:22 UTC by Arfrever Frehtes Taifersar Arahesis
Modified: 2017-04-28 19:22 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2013-10-11 00:00:00
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arfrever Frehtes Taifersar Arahesis 2012-12-30 08:22:54 UTC
$ gcc -xc - -fPIC -shared -o libtest.so <<< ""
$ cat dlerror_test.c
#include <dlfcn.h>
#include <stdio.h>

int main()
{
  void* handle = dlopen("libtest.so", RTLD_NOW);
  dlclose(handle);
  dlclose(handle);
  printf("dlerror(): '%s'\n", dlerror());
}
$ gcc -o dlerror_test dlerror_test.c -ldl
$ LD_LIBRARY_PATH="." ./dlerror_test
dlerror(): '�Ue: shared object not open'
$ LD_LIBRARY_PATH="." ./dlerror_test
dlerror(): '��: shared object not open'
$ LD_LIBRARY_PATH="." ./dlerror_test
dlerror(): '�u�: shared object not open'
$ LD_LIBRARY_PATH="." ./dlerror_test
dlerror(): '��5: shared object not open'
$ LD_LIBRARY_PATH="." ./dlerror_test
dlerror(): '�5�: shared object not open'
$ LD_LIBRARY_PATH="." ./dlerror_test
dlerror(): '�*: shared object not open'
$ LD_LIBRARY_PATH="." ./dlerror_test
dlerror(): '�%�: shared object not open'
$ LD_LIBRARY_PATH="." ./dlerror_test
dlerror(): '�e=: shared object not open'
Comment 1 Rich Felker 2012-12-31 00:20:31 UTC
You invoked undefined behavior by a double-free type error. After the first dlclose, handle is not valid, and you therefore cannot pass it to dlclose again or do anything else with it.
Comment 2 Arfrever Frehtes Taifersar Arahesis 2012-12-31 09:18:23 UTC
Failure of second dlclose() is expected. This bug is about wrong library name in message returned by dlerror(). dlerror() returns correct name of library in case of libc.so.6, but not in case of majority of other libraries.

$ cat dlerror_test.c
#include <dlfcn.h>
#include <stdio.h>
#include <string.h>

int main(int argc, char** argv)
{
  int i;
  for (i = 1; i < argc; i++)
  {
    char buffer[1024];
    snprintf(buffer, 1024, "%s:", argv[i]);
    printf("%-20s", buffer);
    void* handle = dlopen(argv[i], RTLD_NOW);
    if (handle)
    {
      dlclose(handle);
      dlclose(handle);
      printf("dlerror() after double dlclose() : ");
    }
    else
    {
      printf("dlerror() after dlopen()  : ");
    }
    printf("'%s'\n", dlerror());
  }
}
$ gcc -o dlerror_test dlerror_test.c -ldl
$ ./dlerror_test libncurses.so.5 libreadline.so.6 libz.so.1 libc.so.6
libncurses.so.5:    dlerror() after double dlclose() : '0%>: shared object not open'
libreadline.so.6:   dlerror() after double dlclose() : '*>: shared object not open'
libz.so.1:          dlerror() after double dlclose() : 'P%>: shared object not open'
libc.so.6:          dlerror() after double dlclose() : '/lib64/libc.so.6: shared object not open'
Comment 3 Rich Felker 2012-12-31 15:10:36 UTC
No, failure is not expected. Undefined behavior is expected. Undefined behavior is completely different from failure. If your program causes undefined behavior, all bets are off. There is absolutely no requirement on any output of the program, not even output that was logically generated before the undefined behavior was invoked. Programs which invoke undefined behavior are completely invalid and any bug report referring to the behavior of a program that has invoked undefined behavior is invalid.
Comment 4 Arfrever Frehtes Taifersar Arahesis 2013-01-01 10:11:31 UTC
Behavior of dlclose() is defined in POSIX:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/dlclose.html

"RETURN VALUE

If the referenced object was successfully closed, dlclose() shall return 0. If the object could not be closed, or if handle does not refer to an open object, dlclose() shall return a non-zero value. More detailed diagnostic information shall be available through dlerror()."
Comment 5 Rich Felker 2013-01-01 21:27:01 UTC
I've referred the issue to the Austin Group:

http://austingroupbugs.net/view.php?id=639

Calling dlclose on an invalid handle is conceptually just as invalid (i.e. just as much a programming error) as calling free on an invalid pointer, but indeed the current text of the standard seems to specify a behavior for that case which imposes a heavy implementation burden; there's no way to satisfy the requirement without walking the entire list of loaded libraries to check whether handle matches any of them before using it, and I'm not aware of any implementations which do this.

Apologies for responding prematurely without researching the issue.
Comment 6 Rich Felker 2013-01-10 18:56:36 UTC
Austin Group Issue #639 was closed with a resolution that the implementation is required, as currently written, to detect the use of invalid handles. Therefore, glibc is presently non-conforming. All functions which take DSO handles should be updated to walk the loaded DSO list and check that the argument matches the handle of a valid loaded DSO before making any other use of the argument.
Comment 7 Ondrej Bilka 2013-10-11 19:42:03 UTC
This does not change that libraries can be closed unlimited number of times and requiring to keep all names would be equivalent to resource leak. There is currently no quarantee that detailed error will contain accurate name. Keeping a limited history would be a enchancement.

As for compilance a dlsym already does a linear search, see elf/dl_open.c function _dl_find_dso_for_object.

A requirement of returning NULL complicates this, it implies that dlsym/dlclose need to be thread-safe as there would be race. Also on 32bit systems this condition is impossible to fulfill after 1+1<<32 dlopens/dlcloses,

Barring locking a implementation is matter of choosing correct data structure which is in our case perfect hash table. As can choose keys arbitrarily we could just pick first free position and increment last key.

dso **__dso_ary;
int __logsize;
dlsym(long x){
  long hash = (x<<logsize)>>logsize;
  dso *d = __dso_ary[hash];
  if (d->key != x)
    return NULL;
}

dlopen(...){
  long hash=free_pos();
  dso *d = __dso_ary[hash];
  d->key = hash + (d->last_key + 1) % (1<<logsize);
  long x = d->key;
}
Comment 8 Rich Felker 2013-10-11 21:31:53 UTC
I believe it's actually possible for the current code to crash rather than returning a junk string, so this bug should not be closed so quickly. Per the interpretation I cited above (which I agree is unfortunate) an implementation is not permitted to crash if an invalid DSO handle is passed. It must diagnose the error. I would support an effort to have this requirement removed in the next issue of POSIX.
Comment 9 Carlos O'Donell 2017-04-28 00:04:33 UTC
Is it really that hard to implement the lookup required to be able to return an error on a double dlclose()? The standard has required it for a long time, and it does seem useful in many cases where you might not know the state of a handle.
Comment 10 Carlos O'Donell 2017-04-28 00:05:05 UTC
*** Bug 20990 has been marked as a duplicate of this bug. ***
Comment 11 Rich Felker 2017-04-28 16:30:37 UTC
It's essentially never useful for the reason the EBADF error from close is never useful: it can tell you whether the resource identifier was currently valid at the time of the close call, but not whether it referred to what you thought it did, or whether it acted on a resource owned by some other part of the program (or internal to the implementation) that you didn't intend to act on.

The only reason to implement this behavior at all is for strict standards conformance.
Comment 12 Carlos O'Donell 2017-04-28 19:22:32 UTC
(In reply to Rich Felker from comment #11)
> It's essentially never useful for the reason the EBADF error from close is
> never useful: it can tell you whether the resource identifier was currently
> valid at the time of the close call, but not whether it referred to what you
> thought it did, or whether it acted on a resource owned by some other part
> of the program (or internal to the implementation) that you didn't intend to
> act on.

It is useful in specific cases. It requires that program logic allow you to make the kind of assertions your claim.

I agree that in general you can't make abstract assertions about ownership of the handle, but that has it's own problems (infinite memory requirements).

> The only reason to implement this behavior at all is for strict standards
> conformance.

I disagree as stated above. You can use program logic to allow you to make use of the POSIX behaviour.

Knowing if it was _your_ handle, associated with _your_ dlopen, would be really really nice to know. And I think we can do that with monotonically increasing IDs at the expense of ABA on the detection in 32-bit. Though this would only be a hardening feature of an ID-based implementation, and no worse than what POSIX requires.