Re: [bug-gettext] Bug#876498: gettext: msgunfmt: heap corruption

[Bruno Haible]

Hi Daiki,

> Running msgunfmt under valgrind might give you more hints.  Anyway, I am
> suspecting this is caused by a missing NUL termination in
> get_sysdep_string in read-mo.c, which should be fixed by the attached patch.

Yes, the valgrind run points into this direction:
#CHECKER = valgrind --tool=memcheck 
--suppressions=$(abs_srcdir)/../gnulib-lib/malloca.valgrind 
--suppressions=$(abs_srcdir)/../gnulib-lib/libunistring.valgrind 
--num-callers=20 --leak-check=yes --leak-resolution=high --show-reachable=yes
#CHECKER = valgrind --tool=massif --format=html --depth=10 --alloc-fn=xmalloc 
--alloc-fn=xrealloc --stacks=no

Invalid read of size 1
   at 0x4C30A20: __GI_strchr (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x404DC1: read_mo_file (read-mo.c:374)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)
 Address 0x5955ee2 is 0 bytes after a block of size 2 alloc'd
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x44C5D4: xmalloc (xmalloc.c:65)
   by 0x4046F6: get_sysdep_string (read-mo.c:197)
   by 0x404DA6: read_mo_file (read-mo.c:372)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)

Invalid read of size 1
   at 0x4C30F74: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x404E8D: read_mo_file (read-mo.c:392)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)
 Address 0x5955ee2 is 0 bytes after a block of size 2 alloc'd
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x44C5D4: xmalloc (xmalloc.c:65)
   by 0x4046F6: get_sysdep_string (read-mo.c:197)
   by 0x404DA6: read_mo_file (read-mo.c:372)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)

Invalid read of size 1
   at 0x4138D9: format_parse_entrails (format-c-parse.h:199)
   by 0x413CB4: format_parse (format-c.c:68)
   by 0x413D59: format_c_parse (format-c.c:84)
   by 0x404F73: read_mo_file (read-mo.c:414)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)
 Address 0x5955ee2 is 0 bytes after a block of size 2 alloc'd
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x44C5D4: xmalloc (xmalloc.c:65)
   by 0x4046F6: get_sysdep_string (read-mo.c:197)
   by 0x404DA6: read_mo_file (read-mo.c:372)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)

Invalid read of size 1
   at 0x4C30F74: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x404FA1: read_mo_file (read-mo.c:411)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)
 Address 0x5955ee2 is 0 bytes after a block of size 2 alloc'd
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x44C5D4: xmalloc (xmalloc.c:65)
   by 0x4046F6: get_sysdep_string (read-mo.c:197)
   by 0x404DA6: read_mo_file (read-mo.c:372)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)

Invalid read of size 1
   at 0x4138D9: format_parse_entrails (format-c-parse.h:199)
   by 0x413CB4: format_parse (format-c.c:68)
   by 0x413D59: format_c_parse (format-c.c:84)
   by 0x405044: read_mo_file (read-mo.c:432)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)
 Address 0x5956760 is 0 bytes after a block of size 2,096 alloc'd
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x44C5D4: xmalloc (xmalloc.c:65)
   by 0x4046F6: get_sysdep_string (read-mo.c:197)
   by 0x404E70: read_mo_file (read-mo.c:388)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)

Invalid read of size 1
   at 0x4C30F74: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x405072: read_mo_file (read-mo.c:429)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)
 Address 0x5956760 is 0 bytes after a block of size 2,096 alloc'd
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x44C5D4: xmalloc (xmalloc.c:65)
   by 0x4046F6: get_sysdep_string (read-mo.c:197)
   by 0x404E70: read_mo_file (read-mo.c:388)
   by 0x404076: read_one_file (msgunfmt.c:555)
   by 0x403AF8: main (msgunfmt.c:401)

However, I don't agree with the patch. The detailed description of the .mo file
format in gmo.h says:

/* Descriptor for system dependent string.  */
struct sysdep_string
{
  /* Offset of static string segments in file.  */
  nls_uint32 offset;
  /* Alternating sequence of static and system dependent segments.
     The last segment is a static segment, including the trailing NUL.  
<========== */
  struct segment_pair segments[1];
};

So, the last static segment of a system dependent string must end in a NUL
byte. If this NUL byte is present, you don't need to allocate an additional
NUL after it. And if the NUL byte is not present, we better bail out and
signal an error (in read-mo.c) or refuse to load the .mo file (in loadmsgcat.c).

Done through these commits:

http://git.savannah.gnu.org/gitweb/?p=gettext.git;a=commitdiff;h=861745b3c53362925eaf3605a334565af378556c
http://git.savannah.gnu.org/gitweb/?p=gettext.git;a=commitdiff;h=a172ef4d4977eb93f3d23d4d9ed6efac68cd142c

> * gettext-tools/tests/msgunfmt-3: Check no-nul-sysdep.mo.

I like your idea of adding a new test for it. However, I don't like
wildcards when collecting the set of files in a Makefile or test,
because that causes unintended effects when a developer has draft
(uncommitted) changes in his working environment. I find it better
to explicitly enumerate the set of files.

Jakub, thanks for the report. What exactly are you doing to construct these
.mo files? Are you reviewing the code? Are you doing fuzzing?

Bruno