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

[Aaron Watry]

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();

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
>>