Re: Some free()s in module 'relocatable'

[Bruno Haible]

Hi Sylvain,

Sylvain Beucler wrote:
> > > Attached is a small patch to fix a couple non-fred memory blocks. They
> > > are not a big deal, but they do produce noise when analyzing programs
> > > with memory checkers such as Valgrind.
> I do attach the patch now :)

Thanks for the patch. I can even make it work without casts and without
hacks. So I'm applying the modified patch below.

> > > Another thing:, the relocatable module documentation recommends using:
> > >   bindtextdomain (PACKAGE, relocate (LOCALEDIR));
> > > however, the result of relocate may be LOCALEDIR itself (no
> > > relocation) or a newly allocated string.

Yes, this is by design: The relocate() call is meant to be a no-op when
not relocating. People like to use a facility with extra features only when
it costs nothing if the feature is not used.

> Currently I need to use
> tricks to properly free() the result of relocate():
>   default_data_dir = strdup(DEFAULT_DATA_DIR); /* no '!=' with constants */
>   datadir_relocatable = relocate(default_data_dir);
>   ... use datadir_relocatable ...
>   if (datadir_relocatable != default_data_dir)
>     free((char *)datadir_relocatable); /* force non-const variable */

You don't need the strdup; putting DEFAULT_DATA_DIR into a variable is
enough to work around the "no '!=' with constants" problem.

But, more importantly, there's no documentation that says that relocate()
returns either the argument string or a freshly allocated string; it could
also return - say - a pointer into a hidden global obstack. In fact, one
of the return paths of the function (see lib/relocatable.c) returns a
cached global variable that you must not free.


2008-01-01  Sylvain Beucler  <address@hidden>
            Bruno Haible  <address@hidden>

        Improve memory cleanup in 'relocatable' module.
        * lib/relocatable.h (compute_curr_prefix): Change return type to
        'char *'.
        * lib/relocatable.c (compute_curr_prefix): Change return type to
        'char *'. Free curr_installdir after use.
        (relocate): Free curr_prefix_better after use.
        * lib/progreloc.c (prepare_relocate): Free curr_prefix after use.

*** lib/relocatable.h.orig      2008-01-01 20:19:09.000000000 +0100
--- lib/relocatable.h   2008-01-01 16:54:21.000000000 +0100
***************
*** 1,5 ****
  /* Provide relocatable packages.
!    Copyright (C) 2003, 2005 Free Software Foundation, Inc.
     Written by Bruno Haible <address@hidden>, 2003.
  
     This program is free software; you can redistribute it and/or modify it
--- 1,5 ----
  /* Provide relocatable packages.
!    Copyright (C) 2003, 2005, 2008 Free Software Foundation, Inc.
     Written by Bruno Haible <address@hidden>, 2003.
  
     This program is free software; you can redistribute it and/or modify it
***************
*** 59,68 ****
  /* Convenience function:
     Computes the current installation prefix, based on the original
     installation prefix, the original installation directory of a particular
!    file, and the current pathname of this file.  Returns NULL upon failure.  
*/
! extern const char * compute_curr_prefix (const char *orig_installprefix,
!                                        const char *orig_installdir,
!                                        const char *curr_pathname);
  
  #else
  
--- 59,69 ----
  /* Convenience function:
     Computes the current installation prefix, based on the original
     installation prefix, the original installation directory of a particular
!    file, and the current pathname of this file.
!    Returns it, freshly allocated.  Returns NULL upon failure.  */
! extern char * compute_curr_prefix (const char *orig_installprefix,
!                                  const char *orig_installdir,
!                                  const char *curr_pathname);
  
  #else
  
*** lib/relocatable.c.orig      2008-01-01 20:19:09.000000000 +0100
--- lib/relocatable.c   2008-01-01 16:58:10.000000000 +0100
***************
*** 1,5 ****
  /* Provide relocatable packages.
!    Copyright (C) 2003-2006 Free Software Foundation, Inc.
     Written by Bruno Haible <address@hidden>, 2003.
  
     This program is free software; you can redistribute it and/or modify it
--- 1,5 ----
  /* Provide relocatable packages.
!    Copyright (C) 2003-2006, 2008 Free Software Foundation, Inc.
     Written by Bruno Haible <address@hidden>, 2003.
  
     This program is free software; you can redistribute it and/or modify it
***************
*** 160,176 ****
  /* Convenience function:
     Computes the current installation prefix, based on the original
     installation prefix, the original installation directory of a particular
!    file, and the current pathname of this file.  Returns NULL upon failure.  
*/
  #ifdef IN_LIBRARY
  #define compute_curr_prefix local_compute_curr_prefix
  static
  #endif
! const char *
  compute_curr_prefix (const char *orig_installprefix,
                     const char *orig_installdir,
                     const char *curr_pathname)
  {
!   const char *curr_installdir;
    const char *rel_installdir;
  
    if (curr_pathname == NULL)
--- 160,177 ----
  /* Convenience function:
     Computes the current installation prefix, based on the original
     installation prefix, the original installation directory of a particular
!    file, and the current pathname of this file.
!    Returns it, freshly allocated.  Returns NULL upon failure.  */
  #ifdef IN_LIBRARY
  #define compute_curr_prefix local_compute_curr_prefix
  static
  #endif
! char *
  compute_curr_prefix (const char *orig_installprefix,
                     const char *orig_installdir,
                     const char *curr_pathname)
  {
!   char *curr_installdir;
    const char *rel_installdir;
  
    if (curr_pathname == NULL)
***************
*** 254,261 ****
        }
  
      if (rp > rel_installdir)
!       /* Unexpected: The curr_installdir does not end with rel_installdir.  */
!       return NULL;
  
      {
        size_t curr_prefix_len = cp - curr_installdir;
--- 255,265 ----
        }
  
      if (rp > rel_installdir)
!       {
!       /* Unexpected: The curr_installdir does not end with rel_installdir.  */
!       free (curr_installdir);
!       return NULL;
!       }
  
      {
        size_t curr_prefix_len = cp - curr_installdir;
***************
*** 264,274 ****
        curr_prefix = (char *) xmalloc (curr_prefix_len + 1);
  #ifdef NO_XMALLOC
        if (curr_prefix == NULL)
!       return NULL;
  #endif
        memcpy (curr_prefix, curr_installdir, curr_prefix_len);
        curr_prefix[curr_prefix_len] = '\0';
  
        return curr_prefix;
      }
    }
--- 268,283 ----
        curr_prefix = (char *) xmalloc (curr_prefix_len + 1);
  #ifdef NO_XMALLOC
        if (curr_prefix == NULL)
!       {
!         free (curr_installdir);
!         return NULL;
!       }
  #endif
        memcpy (curr_prefix, curr_installdir, curr_prefix_len);
        curr_prefix[curr_prefix_len] = '\0';
  
+       free (curr_installdir);
+ 
        return curr_prefix;
      }
    }
***************
*** 420,434 ****
         orig_prefix.  */
        const char *orig_installprefix = INSTALLPREFIX;
        const char *orig_installdir = INSTALLDIR;
!       const char *curr_prefix_better;
  
        curr_prefix_better =
        compute_curr_prefix (orig_installprefix, orig_installdir,
                             get_shared_library_fullname ());
-       if (curr_prefix_better == NULL)
-       curr_prefix_better = curr_prefix;
  
!       set_relocation_prefix (orig_installprefix, curr_prefix_better);
  
        initialized = 1;
      }
--- 429,447 ----
         orig_prefix.  */
        const char *orig_installprefix = INSTALLPREFIX;
        const char *orig_installdir = INSTALLDIR;
!       char *curr_prefix_better;
  
        curr_prefix_better =
        compute_curr_prefix (orig_installprefix, orig_installdir,
                             get_shared_library_fullname ());
  
!       set_relocation_prefix (orig_installprefix,
!                            curr_prefix_better != NULL
!                            ? curr_prefix_better
!                            : curr_prefix);
! 
!       if (curr_prefix_better != NULL)
!       free (curr_prefix_better);
  
        initialized = 1;
      }
*** lib/progreloc.c.orig        2008-01-01 20:19:09.000000000 +0100
--- lib/progreloc.c     2008-01-01 16:50:32.000000000 +0100
***************
*** 1,5 ****
  /* Provide relocatable programs.
!    Copyright (C) 2003-2007 Free Software Foundation, Inc.
     Written by Bruno Haible <address@hidden>, 2003.
  
     This program is free software: you can redistribute it and/or modify
--- 1,5 ----
  /* Provide relocatable programs.
!    Copyright (C) 2003-2008 Free Software Foundation, Inc.
     Written by Bruno Haible <address@hidden>, 2003.
  
     This program is free software: you can redistribute it and/or modify
***************
*** 281,287 ****
  prepare_relocate (const char *orig_installprefix, const char *orig_installdir,
                  const char *argv0)
  {
!   const char *curr_prefix;
  
    /* Determine the full pathname of the current executable.  */
    executable_fullname = find_executable (argv0);
--- 281,287 ----
  prepare_relocate (const char *orig_installprefix, const char *orig_installdir,
                  const char *argv0)
  {
!   char *curr_prefix;
  
    /* Determine the full pathname of the current executable.  */
    executable_fullname = find_executable (argv0);
***************
*** 290,297 ****
    curr_prefix = compute_curr_prefix (orig_installprefix, orig_installdir,
                                     executable_fullname);
    if (curr_prefix != NULL)
!     /* Now pass this prefix to all copies of the relocate.c source file.  */
!     set_relocation_prefix (orig_installprefix, curr_prefix);
  }
  
  /* Set program_name, based on argv[0], and original installation prefix and
--- 290,301 ----
    curr_prefix = compute_curr_prefix (orig_installprefix, orig_installdir,
                                     executable_fullname);
    if (curr_prefix != NULL)
!     {
!       /* Now pass this prefix to all copies of the relocate.c source file.  */
!       set_relocation_prefix (orig_installprefix, curr_prefix);
! 
!       free (curr_prefix);
!     }
  }
  
  /* Set program_name, based on argv[0], and original installation prefix and