[v2] iconvconfig: Fix multiple issues
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
It was noticed on big-endian systems that msgfmt would fail with the
following error:
msgfmt: gconv_builtin.c:70: __gconv_get_builtin_trans: Assertion `cnt < sizeof (map) / sizeof (map[0])' failed.
Aborted (core dumped)
This is only seen on installed systems because it was due to a
corrupted gconv-modules.cache. iconvconfig had the following issues
(it was specifically freeing fulldir that caused this issue, but other
cleanups are also needed) that this patch fixes.
- Add prefix only if dir starts with '/'
- Use asprintf instead of mempcpy so that the directory string is NULL
terminated
- Make a copy of the directory reference in new_module so that fulldir
can be freed within the same scope in handle_dir.
---
iconv/Makefile | 2 +-
iconv/iconvconfig.c | 24 +++++++++---------------
2 files changed, 10 insertions(+), 16 deletions(-)
Comments
* 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
@@ -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
@@ -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;
}