diff mbox series

iconvconfig: Fix multiple issues

Message ID 20210625060913.300418-1-siddhesh@sourceware.org
State Superseded
Headers
Series 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

Siddhesh Poyarekar June 25, 2021, 6:09 a.m. UTC 
  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 stpcpy instead of mempcpy so that the directory string is NULL
  terminated

- Do not free fulldir because it is used later while writing out the
  gconv-modules.cache file.
---
 iconv/iconvconfig.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Florian Weimer June 25, 2021, 7:26 a.m. UTC | #1 
* Siddhesh Poyarekar via Libc-alpha:

> 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 stpcpy instead of mempcpy so that the directory string is NULL
>   terminated
>
> - Do not free fulldir because it is used later while writing out the
>   gconv-modules.cache file.

I suggest to use asprintf.  This code is hardly performance critical,
and asprintf avoids such bugs.

I expect that static analysis tools can detect the missing free calls,
so I think we should still free those allocations.

Thanks,
Florian
diff mbox series

Patch

diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
index e69334d71c..afc40d06cb 100644
--- a/iconv/iconvconfig.c
+++ b/iconv/iconvconfig.c
@@ -656,7 +656,10 @@  handle_dir (const char *dir)
 
   /* 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 *cp = fulldir;
+  if (dir[0] == '/')
+    cp = mempcpy (cp, prefix, prefix_len);
+  cp = stpcpy (cp, dir);
 
   if (dir[dirlen - 1] != '/')
     {
@@ -667,8 +670,6 @@  handle_dir (const char *dir)
 
   found = gconv_parseconfdir (fulldir, dirlen + prefix_len);
 
-  free (fulldir);
-
   if (!found)
     {
       error (0, errno, "failed to open gconv configuration files in `%s'",