From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32575 invoked by alias); 4 Aug 2004 19:13:44 -0000 Mailing-List: contact libc-hacker-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-hacker-owner@sources.redhat.com Received: (qmail 32442 invoked from network); 4 Aug 2004 19:13:40 -0000 Received: from unknown (HELO sunsite.ms.mff.cuni.cz) (195.113.15.26) by sourceware.org with SMTP; 4 Aug 2004 19:13:40 -0000 Received: from sunsite.ms.mff.cuni.cz (sunsite.mff.cuni.cz [127.0.0.1]) by sunsite.ms.mff.cuni.cz (8.12.8/8.12.8) with ESMTP id i74Gv13j023506; Wed, 4 Aug 2004 18:57:01 +0200 Received: (from jakub@localhost) by sunsite.ms.mff.cuni.cz (8.12.8/8.12.8/Submit) id i74Gv1Kb023504; Wed, 4 Aug 2004 18:57:01 +0200 Date: Wed, 04 Aug 2004 19:13:00 -0000 From: Jakub Jelinek To: Ulrich Drepper Cc: Glibc hackers Subject: [PATCH] Fix various bugs discovered by valgrind Message-ID: <20040804165700.GG30497@sunsite.ms.mff.cuni.cz> Reply-To: Jakub Jelinek Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4.1i X-SW-Source: 2004-08/txt/msg00006.txt.bz2 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 * 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 , 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 , 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 #include #include +#include #include #include @@ -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