Message ID | 20210625090128.316837-1-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
Series | [v2] iconvconfig: Fix multiple issues | expand |
* Siddhesh Poyarekar: > @@ -519,11 +520,12 @@ module_compare (const void *p1, const void *p2) > /* Create new module record. */ > static void > new_module (const char *fromname, size_t fromlen, const char *toname, > - size_t tolen, const char *directory, > + size_t tolen, const char *dir_in, > const char *filename, size_t filelen, int cost, size_t need_ext) > { > struct module *new_module; > - size_t dirlen = strlen (directory) + 1; > + size_t dirlen = strlen (dir_in) + 1; > + const char *directory = xstrdup (dir_in); Sorry, why is this copy needed? Thanks, Florian
On 6/25/21 2:35 PM, Florian Weimer via Libc-alpha wrote: > * Siddhesh Poyarekar: > >> @@ -519,11 +520,12 @@ module_compare (const void *p1, const void *p2) >> /* Create new module record. */ >> static void >> new_module (const char *fromname, size_t fromlen, const char *toname, >> - size_t tolen, const char *directory, >> + size_t tolen, const char *dir_in, >> const char *filename, size_t filelen, int cost, size_t need_ext) >> { >> struct module *new_module; >> - size_t dirlen = strlen (directory) + 1; >> + size_t dirlen = strlen (dir_in) + 1; >> + const char *directory = xstrdup (dir_in); > > Sorry, why is this copy needed? That's the fulldir reference that goes from here into the symtab and so on. Later once the handle_dir function returns and the cache file is being written out, this pointer is referenced and by then it has already been freed. Siddhesh
* Siddhesh Poyarekar: > On 6/25/21 2:35 PM, Florian Weimer via Libc-alpha wrote: >> * Siddhesh Poyarekar: >> >>> @@ -519,11 +520,12 @@ module_compare (const void *p1, const void *p2) >>> /* Create new module record. */ >>> static void >>> new_module (const char *fromname, size_t fromlen, const char *toname, >>> - size_t tolen, const char *directory, >>> + size_t tolen, const char *dir_in, >>> const char *filename, size_t filelen, int cost, size_t need_ext) >>> { >>> struct module *new_module; >>> - size_t dirlen = strlen (directory) + 1; >>> + size_t dirlen = strlen (dir_in) + 1; >>> + const char *directory = xstrdup (dir_in); >> Sorry, why is this copy needed? > > That's the fulldir reference that goes from here into the symtab and > so on. Later once the handle_dir function returns and the cache file > is being written out, this pointer is referenced and by then it has > already been freed. I see, it's returned to the caller via new_module->directory. Apparently we do not free modules, so this doesn't add a new leak. Patch looks okay to me then. Thanks, Florian
diff --git a/iconv/Makefile b/iconv/Makefile index a9b267c851..07d77c9eca 100644 --- a/iconv/Makefile +++ b/iconv/Makefile @@ -33,7 +33,7 @@ vpath %.c ../locale/programs ../intl iconv_prog-modules = iconv_charmap charmap charmap-dir linereader \ dummy-repertoire simple-hash xstrdup xmalloc \ record-status -iconvconfig-modules = strtab xmalloc hash-string +iconvconfig-modules = strtab xmalloc xasprintf xstrdup hash-string extra-objs = $(iconv_prog-modules:=.o) $(iconvconfig-modules:=.o) CFLAGS-iconv_prog.c += -I../locale/programs CFLAGS-iconv_charmap.c += -I../locale/programs diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c index e69334d71c..783b2bbdbb 100644 --- a/iconv/iconvconfig.c +++ b/iconv/iconvconfig.c @@ -250,6 +250,7 @@ static const char gconv_module_ext[] = MODULE_EXT; #include <programs/xmalloc.h> +#include <programs/xasprintf.h> /* C string table handling. */ @@ -519,11 +520,12 @@ module_compare (const void *p1, const void *p2) /* Create new module record. */ static void new_module (const char *fromname, size_t fromlen, const char *toname, - size_t tolen, const char *directory, + size_t tolen, const char *dir_in, const char *filename, size_t filelen, int cost, size_t need_ext) { struct module *new_module; - size_t dirlen = strlen (directory) + 1; + size_t dirlen = strlen (dir_in) + 1; + const char *directory = xstrdup (dir_in); char *tmp; void **inserted; @@ -654,20 +656,10 @@ handle_dir (const char *dir) size_t dirlen = strlen (dir); bool found = false; - /* Add the prefix before sending it off to the parser. */ - char *fulldir = xmalloc (prefix_len + dirlen + 2); - char *cp = mempcpy (mempcpy (fulldir, prefix, prefix_len), dir, dirlen); + char *fulldir = xasprintf ("%s%s%s", dir[0] == '/' ? prefix : "", + dir, dir[dirlen - 1] != '/' ? "/" : ""); - if (dir[dirlen - 1] != '/') - { - *cp++ = '/'; - *cp = '\0'; - dirlen++; - } - - found = gconv_parseconfdir (fulldir, dirlen + prefix_len); - - free (fulldir); + found = gconv_parseconfdir (fulldir, strlen (fulldir)); if (!found) { @@ -679,6 +671,8 @@ handle_dir (const char *dir) "configuration files with names ending in .conf."); } + free (fulldir); + return found ? 0 : 1; }