Bug 983817 - Pad heap allocations passed to flag_qsort() on x86 Linux to work around gcc bug affecting Ubuntu packages. r=froydnj
authorL. David Baron <dbaron@dbaron.org>
Tue, 10 Feb 2015 10:10:49 +1100 (2015-02-09)
changeset 228219 80d3d1eef2f6050ac9334853883ac6d09108bef8
parent 228218 78a06f2602a3374d28bf18da9aaee4b723a258c5
child 228220 5b9ea986e22581387fd2fe2e9d4929be44281b09
push id55351
push userdbaron@mozilla.com
push dateMon, 09 Feb 2015 23:13:34 +0000 (2015-02-09)
treeherdermozilla-inbound@80d3d1eef2f6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs983817
milestone38.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 983817 - Pad heap allocations passed to flag_qsort() on x86 Linux to work around gcc bug affecting Ubuntu packages. r=froydnj 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.cxx
--- a/extensions/spellcheck/hunspell/src/hashmgr.cxx
+++ b/extensions/spellcheck/hunspell/src/hashmgr.cxx
@@ -6,16 +6,30 @@
 #include <stdio.h> 
 #include <ctype.h>
 #include <limits>
 
 #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)
   , userword(0)
   , flag_mode(FLAG_CHAR)
   , complexprefixes(0)
@@ -260,18 +274,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;
@@ -503,33 +517,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;
@@ -543,25 +559,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++;
         }
       }
     }