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

[Ilia Mirkin]

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

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