Bug 983817 - Pad heap allocations passed to flag_qsort() on x86 Linux to work around gcc bug affecting Ubuntu packages. r=froydnj, a=lizzard
authorL. David Baron <dbaron@dbaron.org>
Tue, 10 Feb 2015 10:10:49 +1100
changeset 201094 ae1f4d470fd1e3f8480f04186d86b66fa656233b
parent 201093 8b10f92df3d77f022719ee1a97fea71dfa252db0
child 201095 6db9a07b16f96cf3dfdffa2d3e122aa3abe82701
push id221
push userryanvm@gmail.com
push dateFri, 27 Feb 2015 20:16:40 +0000
treeherdermozilla-esr31@ae1f4d470fd1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, lizzard
bugs983817
milestone31.5.0
Bug 983817 - Pad heap allocations passed to flag_qsort() on x86 Linux to work around gcc bug affecting Ubuntu packages. r=froydnj, a=lizzard My biggest concern for review of this patch is whether the #ifdef will correctly catch what Ubuntu is using to compile Firefox. Does anybody know how to confirm that Ubuntu is compiling with gcc, and that these #ifdefs are correct?
extensions/spellcheck/hunspell/src/hashmgr.cpp
--- a/extensions/spellcheck/hunspell/src/hashmgr.cpp
+++ b/extensions/spellcheck/hunspell/src/hashmgr.cpp
@@ -59,16 +59,30 @@
 #include <string.h>
 #include <stdio.h> 
 #include <ctype.h>
 
 #include "hashmgr.hxx"
 #include "csutil.hxx"
 #include "atypes.hxx"
 
+// The gcc used to build 32-bit builds of Firefox on Ubuntu
+// miscompiles flag_qsort, using a 32-bit read instead of a 16-bit
+// read while quicksorting an array of 16-bit units.  This causes
+// one of the top Firefox crashes.
+// Given that I haven't been able to produce a reduced testcase to give
+// to gcc developers, just work around the bug by allocating an extra 2
+// bytes on the heap arrays passed to flag_qsort().
+// See https://bugzilla.mozilla.org/show_bug.cgi?id=983817 .
+#if defined(__linux__) && defined(__i386__) && defined(__GNUC__)
+#define EXTRA_QSORT_ALLOC_SIZE 1
+#else
+#define EXTRA_QSORT_ALLOC_SIZE 0
+#endif
+
 // build a hash table from a munched word list
 
 HashMgr::HashMgr(const char * tpath, const char * apath, const char * key)
 {
   tablesize = 0;
   tableptr = NULL;
   flag_mode = FLAG_CHAR;
   complexprefixes = 0;
@@ -308,18 +322,18 @@ int HashMgr::get_clen_and_captype(const 
 }
 
 // remove word (personal dictionary function for standalone applications)
 int HashMgr::remove(const char * word)
 {
     struct hentry * dp = lookup(word);
     while (dp) {
         if (dp->alen == 0 || !TESTAFF(dp->astr, forbiddenword, dp->alen)) {
-            unsigned short * flags =
-                (unsigned short *) malloc(sizeof(short) * (dp->alen + 1));
+            unsigned short * flags = (unsigned short *)
+              malloc(sizeof(short) * (dp->alen + 1 + EXTRA_QSORT_ALLOC_SIZE));
             if (!flags) return 1;
             for (int i = 0; i < dp->alen; i++) flags[i] = dp->astr[i];
             flags[dp->alen] = forbiddenword;
             dp->astr = flags;
             dp->alen++;
             flag_qsort(flags, 0, dp->alen);
         }
         dp = dp->next_homonym;
@@ -549,33 +563,35 @@ int HashMgr::decode_flags(unsigned short
         *result = NULL;
         return 0;
     }
     switch (flag_mode) {
       case FLAG_LONG: { // two-character flags (1x2yZz -> 1x 2y Zz)
         len = strlen(flags);
         if (len%2 == 1) HUNSPELL_WARNING(stderr, "error: line %d: bad flagvector\n", af->getlinenum());
         len /= 2;
-        *result = (unsigned short *) malloc(len * sizeof(short));
+        *result = (unsigned short *)
+          malloc((len + EXTRA_QSORT_ALLOC_SIZE) * sizeof(short));
         if (!*result) return -1;
         for (int i = 0; i < len; i++) {
             (*result)[i] = (((unsigned short) flags[i * 2]) << 8) + (unsigned short) flags[i * 2 + 1]; 
         }
         break;
       }
       case FLAG_NUM: { // decimal numbers separated by comma (4521,23,233 -> 4521 23 233)
         int i;
         len = 1;
         char * src = flags; 
         unsigned short * dest;
         char * p;
         for (p = flags; *p; p++) {
           if (*p == ',') len++;
         }
-        *result = (unsigned short *) malloc(len * sizeof(short));
+        *result = (unsigned short *)
+          malloc((len + EXTRA_QSORT_ALLOC_SIZE) * sizeof(short));
         if (!*result) return -1;
         dest = *result;
         for (p = flags; *p; p++) {
           if (*p == ',') {
             i = atoi(src);
             if (i >= DEFAULTFLAGS) HUNSPELL_WARNING(stderr, "error: line %d: flag id %d is too large (max: %d)\n",
               af->getlinenum(), i, DEFAULTFLAGS - 1);
             *dest = (unsigned short) i;
@@ -589,25 +605,27 @@ int HashMgr::decode_flags(unsigned short
           af->getlinenum(), i, DEFAULTFLAGS - 1);
         *dest = (unsigned short) i;
         if (*dest == 0) HUNSPELL_WARNING(stderr, "error: line %d: 0 is wrong flag id\n", af->getlinenum());
         break;
       }    
       case FLAG_UNI: { // UTF-8 characters
         w_char w[BUFSIZE/2];
         len = u8_u16(w, BUFSIZE/2, flags);
-        *result = (unsigned short *) malloc(len * sizeof(short));
+        *result =
+          (unsigned short *) malloc((len + EXTRA_QSORT_ALLOC_SIZE) * sizeof(short));
         if (!*result) return -1;
         memcpy(*result, w, len * sizeof(short));
         break;
       }
       default: { // Ispell's one-character flags (erfg -> e r f g)
         unsigned short * dest;
         len = strlen(flags);
-        *result = (unsigned short *) malloc(len * sizeof(short));
+        *result = (unsigned short *)
+          malloc((len + EXTRA_QSORT_ALLOC_SIZE) * sizeof(short));
         if (!*result) return -1;
         dest = *result;
         for (unsigned char * p = (unsigned char *) flags; *p; p++) {
           *dest = (unsigned short) *p;
           dest++;
         }
       }
     }