Re: [patches] [PATCH] localedef cross endianess problem

[Maxim Kuvyrkov <maxim@xxxxxxxxxxxxxxxx>]

Maxim Kuvyrkov wrote:
> Marc Kleine-Budde wrote:
> > Hello,
> >
> > we're using the stand alone locale def program quite happily, but we
> > noticed a segfault when trying to generate a locale archive for a
> > different endianess (i.e. x64 -> ppc).
> >
> > please keep me on CC, I'm not subscribed.
> >
> > cheers, Marc
> >
> > ---
> >
> > From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> > Subject: fix cross endianess segfault
> >
> > This patch fixes a segfault that occurs if the generated locale
> > archive endianess does not equal the hosts endianess.
> >
> > Without this patch the offset in filedata->strindex is in the
> > wrong endianess which means a quite big offset pointing beyond
> > the allocated data, which causes the later strlen to segfault.
> > This patch swaps the offset to the correct endianess. 
>
> Marc,
>
> Thank you for the patch.  I have a couple of comments on it though:
>
> 1. The problem does not seem to be specific to EGLIBC, the bug is in 
> generic GLIBC code, so the best place to fix it is, probably, directly 
> in FSF GLIBC.  We try not to deviate from FSF GLIBC when unnecessary.

Withdrawn as per Joseph's comment.

> 2. I am in the process of looking at the localedef code and do not yet 
> have a firm understanding of how the data flows; my current 
> understanding, though, is that you're patching up /one/ of the placed 
> where access to ->strindex is made, what about the others?  See, for 
> example, locale.c.
>
> 3. Also, wouldn't it be easier to covert the data to correct endianess 
> at the point of writing instead of the point of reading it?  For what it 
> worth, I haven't yet figured out where the write does occur, so can 
> easily be wrong on this count.

The place where the data is written is locfile.c:write_locale_data(). 
At the start of this function file header ('magic' and 'nstrings' 
fields) and offsets within the file (strindex[]) are converted to target 
endianess.  Hence anytime these data is accessed on host it should be 
converted back.

There are 3 places where the data is being accessed by host: 
locarchive.c:add_locale_to_archive(), locale.c:print_LC_IDENTIFICATION() 
and locale.c:print_LC_CTYPE().  In each of these functions accesses to 
filedata->magic, filedata->nstrings and filedata->strindex[i] fields 
should be wrapped into maybe_swap_uint32().

Marc, your patch handles the place in locarchive.c; the attached patch 
handles the rest.

Joseph, does the analysis and patch look OK to you?

Marc, would you, please, provide the changelog entry for the patch?

Thanks,

--
Maxim K.
CodeSourcery



Index: libc/locale/programs/locarchive.c
===================================================================
--- libc/locale/programs/locarchive.c	(revision 7541)
+++ libc/locale/programs/locarchive.c	(working copy)
@@ -43,6 +43,7 @@
 #include "../localeinfo.h"
 #include "../locarchive.h"
 #include "localedef.h"
+#include "locfile.h"
 
 /* Define the hash function.  We define the function as static inline.
    We must change the name so as not to conflict with simple-hash.h.  */
@@ -973,8 +974,14 @@ add_locale_to_archive (ah, name, data, r
 	unsigned int strindex[0];
       } *filedata = data[LC_CTYPE].addr;
       char *normalized_codeset_name = NULL;
-      codeset = (char *) filedata
-	+ filedata->strindex[_NL_ITEM_INDEX (_NL_CTYPE_CODESET_NAME)];
+      unsigned int strindex;
+
+      /* Data in filedata is stored in target endianess; convert it to
+	 host endianess when needed.  */
+      strindex = filedata->strindex[_NL_ITEM_INDEX (_NL_CTYPE_CODESET_NAME)];
+      strindex = maybe_swap_uint32 (strindex);
+
+      codeset = (char *) filedata + strindex;
 
       normalized_codeset = _nl_normalize_codeset (codeset, strlen (codeset));
       mask |= XPG_NORM_CODESET;
Index: libc/locale/programs/locale.c
===================================================================
--- libc/locale/programs/locale.c	(revision 7541)
+++ libc/locale/programs/locale.c	(working copy)
@@ -43,6 +43,7 @@
 #include "localeinfo.h"
 #include "charmap-dir.h"
 #include "../locarchive.h"
+#include "locfile.h"
 
 extern void *xmalloc (size_t __n);
 extern char *xstrdup (const char *__str);
@@ -332,18 +333,22 @@ print_LC_IDENTIFICATION (void *mapped, s
       unsigned int strindex[0];
     } *filedata = mapped;
 
-  if (filedata->magic == LIMAGIC (LC_IDENTIFICATION)
+  /* Data in filedata is stored in target endianess; convert it to
+     host endianess when needed.  */
+
+  if (maybe_swap_uint32 (filedata->magic) == LIMAGIC (LC_IDENTIFICATION)
       && (sizeof *filedata
-	  + (filedata->nstrings
+	  + (maybe_swap_uint32 (filedata->nstrings)
 	     * sizeof (unsigned int))
 	  <= size))
     {
       const char *str;
 
 #define HANDLE(idx, name) \
-  str = ((char *) mapped						      \
-	 + filedata->strindex[_NL_ITEM_INDEX (_NL_IDENTIFICATION_##idx)]);    \
-  if (*str != '\0')							      \
+  str = ((char *) mapped						       \
+	 + (maybe_swap_uint32						       \
+	    (filedata->strindex[_NL_ITEM_INDEX (_NL_IDENTIFICATION_##idx)]))); \
+  if (*str != '\0')							       \
     printf ("%9s | %s\n", name, str)
       HANDLE (TITLE, "title");
       HANDLE (SOURCE, "source");
@@ -373,16 +378,20 @@ print_LC_CTYPE (void *mapped, size_t siz
       unsigned int strindex[0];
     } *filedata = mapped;
 
-  if (filedata->magic == LIMAGIC (LC_CTYPE)
+  /* Data in filedata is stored in target endianess; convert it to
+     host endianess when needed.  */
+
+  if (maybe_swap_uint32 (filedata->magic) == LIMAGIC (LC_CTYPE)
       && (sizeof *filedata
-	  + (filedata->nstrings
+	  + (maybe_swap_uint32 (filedata->nstrings)
 	     * sizeof (unsigned int))
 	  <= size))
     {
       const char *str;
 
       str = ((char *) mapped
-	     + filedata->strindex[_NL_ITEM_INDEX (_NL_CTYPE_CODESET_NAME)]);
+	     + (maybe_swap_uint32
+		(filedata->strindex[_NL_ITEM_INDEX (_NL_CTYPE_CODESET_NAME)])));
       if (*str != '\0')
 	printf ("  codeset | %s\n", str);
     }