[Mesa-dev] [PATCH 4/4] aux/pipe_loader: Don't leak dlerror string on dlopen failure

[Aaron Watry]

On Thu, Nov 13, 2014 at 7:04 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Thu, Nov 13, 2014 at 7:54 PM, Aaron Watry <awatry at gmail.com> wrote:
>>
>> On Thu, Nov 13, 2014 at 6:22 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> > On Thu, Nov 13, 2014 at 6:43 PM, Aaron Watry <awatry at gmail.com> wrote:
>> >> dlopen allocates a string on dlopen failure which is retrieved via dlerror. In
>> >> order to free that string, you need to retrieve and then free it.
>> >
>> > Are you basically saying that glibc leaks memory and you're trying to
>> > make up for it? What if you use a non-buggy library? Or is dlopen()
>> > specified in such a way that if it fails, you must free the result of
>> > dlerror? I see nothing in the man pages to suggest that...
>>
>> The closest that I can come to documentation at least implying this is [1]:
>>
>> "
>> RETURN VALUE
>>
>> If file cannot be found, cannot be opened for reading, is not of an
>> appropriate object format for processing by dlopen(), or if an error
>> occurs during the process of loading file or relocating its symbolic
>> references, dlopen() shall return NULL. More detailed diagnostic
>> information shall be available through dlerror().
>> "
>>
>> Which implies that libdl needs to keep some sort of state regarding
>> the last error encountered.  I see no requirement that it keep a
>> malloc'd string, just that it keep some state information around.
>>
>> [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/dlopen.html
>>
>> That does seem to lead one to read the dlerror() page that has this gem:
>> "
>> DESCRIPTION
>>
>> The dlerror() function shall return a null-terminated character string
>> (with no trailing <newline>) that describes the last error that
>> occurred during dynamic linking processing. If no dynamic linking
>> errors have occurred since the last invocation of dlerror(), dlerror()
>> shall return NULL. Thus, invoking dlerror() a second time, immediately
>> following a prior invocation, shall result in NULL being returned.
>>
>> <snip>
>>
>> APPLICATION USAGE
>> The messages returned by dlerror() may reside in a static buffer that
>> is overwritten on each call to dlerror()
>> "
>>
>> So, it may or may not return a malloc'd string, and all I've managed
>> here is to fix a leak in glibc's specific implementation.
>>
>> The above docs seem to imply that dlopen() triggering an error needs
>> to populate some state and dlerror() retrieves the last error that has
>> occurred since the last dlerror() call. calling dlerror() again at
>> that point will return null.
>>
>> That being said, I think a simpler fix and probably more correct fix
>> would be to do:
>> dlerror(); dlerror();
>
> That seems like it's vastly less likely to do the wrong thing with any
> reasonable implementation. So if it fixes things for glibc, let's do
> that instead :) Also, I wonder, even if glibc malloc's the string,
> whether it is truly leaked or valgrind just thinks that. May be
> interesting to pull the curtain back and see what's actually going on
> in glibc...
>

According to glibc's current source, it does actually malloc the error
string and keep it around in either a static or thread local struct
with a pointer to the error string depending on how it's built.

There's comments in the source implying that if a dlopen fails, it's
recommended to then run dlerror() to see why it failed...  If you do
happen to dlopen() with failure multiple times in a row, it does only
leak the 1 error string (it cleans up after itself every time you call
dlopen() before it actually attempts to open the new library)..  But
in general, that malloc'd string will hang around until the next
dlerror() call or the next time you call another dl*() function.  We
could have the same issue with dlsym, dlclose, etc.

https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=dlfcn/dlopen.c;hb=HEAD
https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=dlfcn/dlerror.c;hb=HEAD

So for now, I'll just change this entire patch to call dlerror() twice
instead of getting the error and then explicitly freeing it (which
really was wrong.  My bad.). The other file (u_dl.c) will remain as is
(no changes in this patch).

--Aaron

>   -ilia
>
>>
>>
>> Thoughts?
>>
>> --Aaron
>>
>>
>>
>> >
>> >>
>> >> In order to keep things legit the windows/other util_dl_error paths allocate
>> >> and then copy their error message into a buffer as well.
>> >>
>> >> Signed-off-by: Aaron Watry <awatry at gmail.com>
>> >> CC: Ilia Mirkin <imirkin at alum.mit.edu>
>> >>
>> >> v3: Switch comment to C-Style
>> >> v2: Use strdup instead of calloc/strcpy
>> >> ---
>> >>  src/gallium/auxiliary/pipe-loader/pipe_loader.c | 5 +++++
>> >>  src/gallium/auxiliary/util/u_dl.c               | 4 ++--
>> >>  2 files changed, 7 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/src/gallium/auxiliary/pipe-loader/pipe_loader.c b/src/gallium/auxiliary/pipe-loader/pipe_loader.c
>> >> index 8e79f85..7a4e0b1 100644
>> >> --- a/src/gallium/auxiliary/pipe-loader/pipe_loader.c
>> >> +++ b/src/gallium/auxiliary/pipe-loader/pipe_loader.c
>> >> @@ -25,6 +25,8 @@
>> >>   *
>> >>   **************************************************************************/
>> >>
>> >> +#include <dlfcn.h>
>> >> +
>> >>  #include "pipe_loader_priv.h"
>> >>
>> >>  #include "util/u_inlines.h"
>> >> @@ -101,6 +103,9 @@ pipe_loader_find_module(struct pipe_loader_device *dev,
>> >>           if (lib) {
>> >>              return lib;
>> >>           }
>> >> +
>> >> +         /* Retrieve the dlerror() str so that it can be freed properly */
>> >> +         FREE(util_dl_error());
>> >>        }
>> >>     }
>> >>
>> >> diff --git a/src/gallium/auxiliary/util/u_dl.c b/src/gallium/auxiliary/util/u_dl.c
>> >> index aca435d..00c4d7c 100644
>> >> --- a/src/gallium/auxiliary/util/u_dl.c
>> >> +++ b/src/gallium/auxiliary/util/u_dl.c
>> >> @@ -87,8 +87,8 @@ util_dl_error(void)
>> >>  #if defined(PIPE_OS_UNIX)
>> >>     return dlerror();
>> >>  #elif defined(PIPE_OS_WINDOWS)
>> >> -   return "unknown error";
>> >> +   return strdup("unknown error");
>> >>  #else
>> >> -   return "unknown error";
>> >> +   return strdup("unknown error");
>> >>  #endif
>> >>  }
>> >> --
>> >> 2.1.0
>> >>