This is the mail archive of the libc-hacker@sources.redhat.com mailing list for the glibc project.

Note that libc-hacker is a closed list. You may look at the archives of this list, but subscription and posting are not open.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix various bugs discovered by valgrind


Hi!

I have modified Makeconfig after doing make (but before make check) to:
run-program-prefix = $(if $(filter $(notdir $(built-program-file)),\
                                   $(tests-static)),, \
                          valgrind --tool=memcheck)

This patch fixes some of the issues it discovered during make check run.

The first two ChangeLog entries are because struct printf_info's i18n
member (and in one case is_char too) was not initialized.  Rather than
leaving there a ticking bomb for the next time another field is added
to the structure, clearing resp. initializing it as whole is IMHO much
better.  For printf_size.c it also decreases the size of the routine
on x86-64 by around 140 bytes (which just means GCC doesn't handle
bitfields as good as it could).

The nscd change is I guess obvious, a pointer is hardly > 7 and < 11.

Not all ld-*.c readers initialize return_widestr right away, so get_line
then looks at unitialized value.  We were apparently lucky, I have verified
it generates the same localedata/* test locales as before.

There are a couple of __libc_freeres_fn_section additions for routines
which are solely called from freeres hooks.  While looking at them,
I stopped on dl-close.c free_mem, where IMHO we can't call free_slotinfo
twice.  The first free_slotinfo sets GL(dl_tls_dtv_slotinfo_list)
to NULL (after freeing it and its chain), so we can't call
free_slotinfo (&((struct dtv_slotinfo_list *) NULL)->next) then.

tst-fmemopen is obvious too, mmap fails with MAP_FAILED, not NULL.

The last changes fix the largest amount of valgrind failures.
free_derivation called from gconv_db.c's free_mem frees the steps
arrays, so if setlocale.c's free_mem (which calls indirectly
__gconv_close_transform) is run after it, it walks over a freed arrays.

make check with valgrind's memcheck on glibc with this patch is still
running, but on the tests that were run so far, without this patch
I had previously 328 programs which reported errors in valgrind, now
only 8.

2004-08-04  Jakub Jelinek  <jakub@redhat.com>

	* stdlib/strfmon_l.c (__vstrfmon_l): Memset whole info structure
	instead of trying to initialize some, but not all, fields one by
	one.
	* stdio-common/printf_size.c (printf_size): Initialize fb_info
	structure with *info instead of trying to initialize some, but not
	all, fields from it.

	* nscd/connections.c (handle_request): Check if req->type is in
	LASTDBREQ .. LASTREQ range instead of req.

	* locale/programs/linereader.c (lr_create): Initialize
	lr->return_widestr to 0.

	* elf/dl-close.c (free_slotinfo): Add __libc_freeres_fn_section.
	(free_mem): Call free_slotinfo just once.

	* stdio-common/tst-fmemopen.c (main): Check for MAP_FAILED instead
	of NULL.

	* locale/localeinfo.h (_nl_locale_subfreeres): New prototype.
	* locale/setlocale.c (free_category): Add __libc_freeres_fn_section.
	(free_mem): Rename to...
	(_nl_locale_subfreeres).
	* iconv/gconv_db.c: Include locale/localeinfo.h.
	(free_derivation, free_modules_db): Add __libc_freeres_fn_section.
	(free_mem): Call _nl_locale_subfreeres.
	* iconv/gconv_dl.c (do_release_all): Add __libc_freeres_fn_section.

--- libc/stdlib/strfmon_l.c.jj	2004-08-04 14:42:24.000000000 +0200
+++ libc/stdlib/strfmon_l.c	2004-08-04 15:29:41.713238326 +0200
@@ -543,20 +543,14 @@ __vstrfmon_l (char *s, size_t maxsize, _
 	 the numeric representation is too long.  */
       s[maxsize - 1] = '\0';
 
+      memset (&info, '\0', sizeof (info));
       info.prec = right_prec;
       info.width = left_prec + (right_prec ? (right_prec + 1) : 0);
       info.spec = 'f';
       info.is_long_double = is_long_double;
-      info.is_short = 0;
-      info.is_long = 0;
-      info.alt = 0;
-      info.space = 0;
-      info.left = 0;
-      info.showsign = 0;
       info.group = group;
       info.pad = pad;
       info.extra = 1;		/* This means use values from LC_MONETARY.  */
-      info.wide = 0;
 
       ptr = &fpnum;
       done = __printf_fp ((FILE *) &f, &info, &ptr);
--- libc/nscd/connections.c.jj	2004-08-04 14:42:22.000000000 +0200
+++ libc/nscd/connections.c	2004-08-04 15:50:05.000000000 +0200
@@ -342,7 +342,7 @@ cannot handle old request version %d; cu
     {
       if (req->type == INVALIDATE)
 	dbg_log ("\t%s (%s)", serv2str[req->type], (char *)key);
-      else if (req > LASTDBREQ && req < LASTREQ)
+      else if (req->type > LASTDBREQ && req->type < LASTREQ)
 	dbg_log ("\t%s", serv2str[req->type]);
       else
 	dbg_log (_("\tinvalid request type %d"), req->type);
--- libc/elf/dl-close.c.jj	2004-03-08 12:01:41.000000000 +0100
+++ libc/elf/dl-close.c	2004-08-04 16:33:40.064983561 +0200
@@ -565,7 +565,7 @@ libc_hidden_def (_dl_close)
 
 
 #ifdef USE_TLS
-static bool
+static bool __libc_freeres_fn_section
 free_slotinfo (struct dtv_slotinfo_list **elemp)
 {
   size_t cnt;
@@ -623,11 +623,12 @@ libc_freeres_fn (free_mem)
 	/* There was no initial TLS setup, it was set up later when
 	   it used the normal malloc.  */
 	free_slotinfo (&GL(dl_tls_dtv_slotinfo_list));
+      else
 # endif
-      /* The first element of the list does not have to be deallocated.
-	 It was allocated in the dynamic linker (i.e., with a different
-	 malloc), and in the static library it's in .bss space.  */
-      free_slotinfo (&GL(dl_tls_dtv_slotinfo_list)->next);
+        /* The first element of the list does not have to be deallocated.
+	   It was allocated in the dynamic linker (i.e., with a different
+	   malloc), and in the static library it's in .bss space.  */
+	free_slotinfo (&GL(dl_tls_dtv_slotinfo_list)->next);
     }
 #endif
 }
--- libc/locale/programs/linereader.c.jj	2003-06-11 23:53:21.000000000 +0200
+++ libc/locale/programs/linereader.c	2004-08-04 17:22:20.220747885 +0200
@@ -1,4 +1,4 @@
-/* Copyright (C) 1996-2001, 2002, 2003 Free Software Foundation, Inc.
+/* Copyright (C) 1996-2001, 2002, 2003, 2004 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@gnu.org>, 1996.
 
@@ -80,6 +80,7 @@ lr_create (FILE *fp, const char *fname, 
   result->comment_char = '#';
   result->escape_char = '\\';
   result->translate_strings = 1;
+  result->return_widestr = 0;
 
   n = getdelim (&result->buf, &result->bufsize, '\n', result->fp);
   if (n < 0)
--- libc/locale/localeinfo.h.jj	2003-11-18 11:34:25.000000000 +0100
+++ libc/locale/localeinfo.h	2004-08-04 19:24:11.530237976 +0200
@@ -315,6 +315,9 @@ extern struct locale_data *_nl_load_loca
 /* Subroutine of setlocale's __libc_subfreeres hook.  */
 extern void _nl_archive_subfreeres (void) attribute_hidden;
 
+/* Subroutine of gconv-db's __libc_subfreeres hook.  */
+extern void _nl_locale_subfreeres (void) attribute_hidden;
+
 /* Validate the contents of a locale file and set up the in-core
    data structure to point into the data.  This leaves the `alloc'
    and `name' fields uninitialized, for the caller to fill in.
--- libc/locale/setlocale.c.jj	2004-01-12 10:52:35.000000000 +0100
+++ libc/locale/setlocale.c	2004-08-04 19:23:34.788676862 +0200
@@ -1,4 +1,5 @@
-/* Copyright (C) 1991, 92, 1995-2000, 2002, 2003 Free Software Foundation, Inc.
+/* Copyright (C) 1991, 1992, 1995-2000, 2002, 2003, 2004
+   Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -444,7 +445,7 @@ setlocale (int category, const char *loc
 }
 libc_hidden_def (setlocale)
 
-static void
+static void __libc_freeres_fn_section
 free_category (int category,
 	       struct locale_data *here, struct locale_data *c_data)
 {
@@ -472,7 +473,10 @@ free_category (int category,
     }
 }
 
-libc_freeres_fn (free_mem)
+/* This is called from iconv/gconv_db.c's free_mem, as locales must
+   be freed before freeing gconv steps arrays.  */
+void __libc_freeres_fn_section
+_nl_locale_subfreeres (void)
 {
 #ifdef NL_CURRENT_INDIRECT
   /* We don't use the loop because we want to have individual weak
--- libc/stdio-common/printf_size.c.jj	2004-08-04 14:42:24.000000000 +0200
+++ libc/stdio-common/printf_size.c	2004-08-04 15:44:45.767792807 +0200
@@ -201,18 +201,9 @@ printf_size (FILE *fp, const struct prin
 
   /* Prepare to print the number.  We want to use `__printf_fp' so we
      have to prepare a `printf_info' structure.  */
+  fp_info = *info;
   fp_info.spec = 'f';
   fp_info.prec = info->prec < 0 ? 3 : info->prec;
-  fp_info.is_long_double = info->is_long_double;
-  fp_info.is_short = info->is_short;
-  fp_info.is_long = info->is_long;
-  fp_info.alt = info->alt;
-  fp_info.space = info->space;
-  fp_info.left = info->left;
-  fp_info.showsign = info->showsign;
-  fp_info.group = info->group;
-  fp_info.extra = info->extra;
-  fp_info.pad = info->pad;
   fp_info.wide = wide;
 
   if (fp_info.left && fp_info.pad == L' ')
--- libc/stdio-common/tst-fmemopen.c.jj	2000-11-20 18:39:23.000000000 +0100
+++ libc/stdio-common/tst-fmemopen.c	2004-08-04 17:44:46.224206273 +0200
@@ -62,7 +62,7 @@ main (void)
     exit (4);
 
   if ((mmap_data = (char *) mmap (NULL, fs.st_size, PROT_READ,
-				  MAP_SHARED, fd, 0)) == NULL)
+				  MAP_SHARED, fd, 0)) == MAP_FAILED)
     {
       if (errno == ENOSYS)
 	exit (0);
--- libc/iconv/gconv_dl.c.jj	2003-11-12 09:50:52.000000000 +0100
+++ libc/iconv/gconv_dl.c	2004-08-04 16:20:07.403351309 +0200
@@ -1,5 +1,6 @@
 /* Handle loading/unloading of shared object for transformation.
-   Copyright (C) 1997,1998,1999,2000,2001,2002 Free Software Foundation, Inc.
+   Copyright (C) 1997, 1998, 1999, 2000, 2001, 2002, 2004
+   Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@cygnus.com>, 1997.
 
@@ -191,7 +192,7 @@ __gconv_release_shlib (struct __gconv_lo
 
 
 /* We run this if we debug the memory allocation.  */
-static void
+static void __libc_freeres_fn_section
 do_release_all (void *nodep)
 {
   struct __gconv_loaded_object *obj = (struct __gconv_loaded_object *) nodep;
--- libc/iconv/gconv_db.c.jj	2004-03-10 10:31:58.000000000 +0100
+++ libc/iconv/gconv_db.c	2004-08-04 19:28:50.190405080 +0200
@@ -24,6 +24,7 @@
 #include <string.h>
 #include <sys/param.h>
 #include <bits/libc-lock.h>
+#include <locale/localeinfo.h>
 
 #include <dlfcn.h>
 #include <gconv_int.h>
@@ -170,7 +171,7 @@ add_derivation (const char *fromset, con
      not all memory will be freed.  */
 }
 
-static void
+static void __libc_freeres_fn_section
 free_derivation (void *p)
 {
   struct known_derivation *deriv = (struct known_derivation *) p;
@@ -765,7 +766,7 @@ __gconv_close_transform (struct __gconv_
 
 /* Free the modules mentioned.  */
 static void
-internal_function
+internal_function __libc_freeres_fn_section
 free_modules_db (struct gconv_module *node)
 {
   if (node->left != NULL)
@@ -786,6 +787,10 @@ free_modules_db (struct gconv_module *no
 /* Free all resources if necessary.  */
 libc_freeres_fn (free_mem)
 {
+  /* First free locale memory.  This needs to be done before freeing derivations,
+     as ctype cleanup functions dereference steps arrays which we free below.  */
+  _nl_locale_subfreeres ();
+
   if (__gconv_alias_db != NULL)
     __tdestroy (__gconv_alias_db, free);
 


	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]