Bug 983817 - Pad heap allocations passed to flag_qsort() on x86 Linux to work around gcc bug affecting Ubuntu packages. r=froydnj, a=sledru
authorL. David Baron <dbaron@dbaron.org>
Tue, 10 Feb 2015 10:10:49 +1100
changeset 243755 ca56ab5d9989
parent 243754 61a56699e22d
child 243756 9295bf4a3442
push id4464
push userryanvm@gmail.com
push date2015-02-11 16:02 +0000
treeherdermozilla-beta@69a8d311ddd6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, sledru
bugs983817
milestone36.0
Bug 983817 - Pad heap allocations passed to flag_qsort() on x86 Linux to work around gcc bug affecting Ubuntu packages. r=froydnj, a=sledru 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++;
         }
       }
     }