Bug 506952 (f12ffcrash)
Summary: | xulrunner-1.9.1-0.22.beta4.fc12.x86_64 SIGABRTs firefox | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Tom London <selinux> | ||||||||||||
Component: | xulrunner | Assignee: | Gecko Maintainer <gecko-bugs-nobody> | ||||||||||||
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||||||||||
Severity: | high | Docs Contact: | |||||||||||||
Priority: | high | ||||||||||||||
Version: | rawhide | CC: | b1r63r, boutilpj, caillon, gecko-bugs-nobody, greenrd, igor.zubkov, jakub, jik, john.ellson, johnp, mattdm, mcepl, nicolas.mailhot, paul, redhat, ruslanpisarev, rwu, sg266, stransky, walters, yaneti, yunus.tji.nyan | ||||||||||||
Target Milestone: | --- | Keywords: | Reopened | ||||||||||||
Target Release: | --- | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | Linux | ||||||||||||||
Whiteboard: | |||||||||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||||||||
Doc Text: | Story Points: | --- | |||||||||||||
Clone Of: | Environment: | ||||||||||||||
Last Closed: | 2009-07-07 09:26:07 UTC | Type: | --- | ||||||||||||
Regression: | --- | Mount Type: | --- | ||||||||||||
Documentation: | --- | CRM: | |||||||||||||
Verified Versions: | Category: | --- | |||||||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||||||
Embargoed: | |||||||||||||||
Attachments: |
|
Description
Tom London
2009-06-19 14:04:10 UTC
Confirmed. Same problem here. Confirmed in 32-bit. *** Bug 507118 has been marked as a duplicate of this bug. *** There are rumours (http://thread.gmane.org/gmane.linux.redhat.fedora.testers/73468), that this is a gcc problem. Jakub? Rumors, no proofs. What is true is that I've extended -D_FORTIFY_SOURCE to check cases it didn't check before. In gcc-4.4.0-{7,8} there has been a bug in it, so if a package has been built with one of these, it is very possible it fails and needs to be recompiled. But if it still happens after being recompiled with gcc-4.4.0-9 and above, then it is either a package bug, or of course could still be a gcc bug. I can look at it, but need for that: 1) preprocessed source in which this happens (in the case above jsscript.cpp) 2) exact gcc options it has been compiled with 3) some info from the debugger, how long the string being copied is Kernel AFAIK doesn't use -D_FORTIFY_SOURCE (or has that changed?), so this shouldn't have anything to do with the kernel, and there haven't been any substantial changes in gcc since F11. Hm, seems to be here: /* NB: This struct overlays JSHashEntry -- see jshash.h, do not reorganize. */ typedef struct ScriptFilenameEntry { JSHashEntry *next; /* hash chain linkage */ JSHashNumber keyHash; /* key hash function result */ const void *key; /* ptr to filename, below */ uint32 flags; /* user-defined filename prefix flags */ JSPackedBool mark; /* GC mark flag */ char filename[3]; /* two or more bytes, NUL-terminated */ } ScriptFilenameEntry; in jsscript.cpp: ScriptFilenameEntry *sfe; 1104: sfe->key = strcpy(sfe->filename, filename); Can strcpy -> strncpy switch fix the fortify issue? This is not a self-contained testcase. For struct S { void *p; int j; char b[3]; }; struct S *s; int foo (void) { return __builtin_object_size (s->b, 1); } gcc -O2 returns (size_t) -1, i.e. it doesn't know anything. For int bar (void) { struct S *t = __builtin_malloc (sizeof (struct S)); return __builtin_object_size (t->b, 1); } it returns 4 (still not 3, as the last field is assumed to be kind of poor man's flexible array member and there is 1 byte padding at the end of the struct). It is important to know if sfe above is an automatic variable or not and if gcc has a chance to find out what it points to and whether it could have been clobbered somewhere between the initialization and where object size is queried (e.g. from strcpy). ScriptFilenameEntry *sfe is an automatic variable. The code is: sfe = (ScriptFilenameEntry *) JS_HashTableRawAdd(table, hep, hash, filename, NULL); if (!sfe) return NULL; sfe->key = strcpy(sfe->filename, filename); sfe->flags = 0; sfe->mark = JS_FALSE; and: JS_HashTableRawAdd(JSHashTable *ht, JSHashEntry **hep, JSHashNumber keyHash, const void *key, void *value) { [...] /* Make a new key value entry */ he = ht->allocOps->allocEntry(ht->allocPriv, key); if (!he) return NULL; he->keyHash = keyHash; he->key = key; he->value = value; he->next = *hep; *hep = he; ht->nentries++; return he; } I'm going to find out what's behind "allocOps->allocEntry()", looks like some private allocator. Just to make sure, are you sure you can reproduce it when this code is compiled with gcc 4.4.0-9? I can't reproduce it with my current 4.4.0-4 (F11). Shall I update it and retest? ht->allocOps->allocEntry returns pointers to memory areas allocated by malloc. Just check if the rawhide xulrunner or whatever else owns the library that called strcpy which triggered __fortify_fail has been built with gcc-4.4.0-{7,8} (koji should know, look at root.log) and if yes, rebuild it. The affected package (xulrunner*-1.9.1-0.22.beta4.fc12.x86_64) was built with gcc 4.4.0-9 (http://kojipkgs.fedoraproject.org/packages/xulrunner/1.9.1/0.22.beta4.fc12/data/logs/i586/root.log) The working one (xulrunner*-1.9.1-0.20.beta4.fc12.x86_64) was built with gcc 4.4.0-3 (http://kojipkgs.fedoraproject.org/packages/xulrunner/1.9.1/0.20.beta4.fc12/data/logs/i586/root.log) In that case, please attach preprocessed source and exact gcc cmdline options used to compile it. I'll have a look. And, try to find out how long the filename passed to strcpy actually is. *** Bug 507345 has been marked as a duplicate of this bug. *** Created attachment 348895 [details]
proprocessed jshash.cpp
Created attachment 348896 [details]
preprocessed jsscript.cpp
Created attachment 348897 [details]
prep. jsatom.cpp
Created attachment 348898 [details]
prep. jsarea.cpp
ht->allocOps->allocEntry can points to: js_alloc_sftbl_entry js_alloc_temp_entry ----------------------------------------------------- js_alloc_sftbl_entry(void *priv, const void *key) { size_t nbytes = __builtin_offsetof (ScriptFilenameEntry, filename) + strlen((const char *) key) + 1; return (JSHashEntry *) malloc(((nbytes)>(sizeof(JSHashEntry))?(nbytes):(sizeof(JSHashEntry)))); } ----------------------------------------------------- static JSHashEntry * js_alloc_temp_entry(void *priv, const void *key) { JSCompiler *jsc = (JSCompiler *) priv; JSAtomListElement *ale; ale = jsc->aleFreeList; if (ale) { jsc->aleFreeList = ((JSAtomListElement *) (ale)->entry.next); return &ale->entry; } do { JSArena *_a = (&jsc->context->tempPool)->current; size_t _nb = (((jsuword)(sizeof(JSAtomListElement)) + (&jsc->context->tempPool)->mask) & ~(&jsc->context->tempPool)->mask); jsuword _p = _a->avail; if ((0) || _p > _a->limit - _nb) _p = (jsuword)JS_ArenaAllocate(&jsc->context->tempPool, _nb); else _a->avail = _p + _nb; ale = (JSAtomListElement *) _p; ; } while (0); if (!ale) { js_ReportOutOfScriptQuota(jsc->context); return __null; } return &ale->entry; } and JS_ArenaAllocate() is in jsarena file: __attribute__((visibility ("default"))) void * JS_ArenaAllocate(JSArenaPool *pool, size_t nb); ----------------------------------------------------- gcc i586 flags: c++ -fPIC-fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i586 -mtune=generic -fasynchronous-unwind-tables -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -Os -fstrict-aliasing Still not sure whether it is gcc or firefox bug, but certainly it is way above bug triaging. *** Bug 507372 has been marked as a duplicate of this bug. *** Ah, typedef __SIZE_TYPE__ size_t; typedef struct S { int i, j, k; char buf[3]; } T; __attribute__((noinline)) void foo (T *t) { if (__builtin_object_size (&t->buf[0], 1) != (size_t) -1) __builtin_abort (); if (__builtin_object_size (&t->buf, 1) != (size_t) -1) __builtin_abort (); } int main () { T t; foo (&t); } at -O2 only works with gcc, but not g++, as C++ inserts TYPE_DECL after the buf's FIELD_DECL. So the check for last field should just walk that FIELD_DECL's TREE_CHAIN, looking for FIELD_DECLs, instead of checking that TREE_CHAIN is non-NULL. Rather than converting this into a gcc bug, shouldn't there be a separate gcc bug, with the xulrunner bug remaining as an xulrunner bug so that we remember to recompile a new xulrunner RPM when the gcc bug is fixed? Or is it moot because we're going to have to recompile everything once the gcc bug is fixed? (In reply to comment #25) > Rather than converting this into a gcc bug, shouldn't there be a separate gcc > bug, with the xulrunner bug remaining as an xulrunner bug so that we remember > to recompile a new xulrunner RPM when the gcc bug is fixed? Don't worry that Firefox maintainers are watching this bug carefully. They are. (In reply to comment #25) > Rather than converting this into a gcc bug, shouldn't there be a separate gcc > bug, with the xulrunner bug remaining as an xulrunner bug so that we remember > to recompile a new xulrunner RPM when the gcc bug is fixed? Don't worry that Firefox maintainers are not watching this bug carefully. They are. Please rebuild xulrunner with gcc-4.4.0-10 in rawhide. rebuild in xulrunner-1.9.1-0.23.beta4.fc12 fixed the startup crash for me but I get reproducible fortify fail opening this particular bugreport. (Logged in , hence with the comments box present and that hunspell stuff you can see below) ...... #3 0x0000003ccf6f7537 in *__GI___fortify_fail ( msg=0x3ccf73436f "buffer overflow detected") at fortify_fail.c:32 #4 0x0000003ccf6f5590 in *__GI___chk_fail () at chk_fail.c:29 #5 0x00007ffff1537c12 in strcpy (__src=<value optimized out>, __dest=<value optimized out>) at /usr/include/bits/string3.h:106 #6 HashMgr::add_word (__src=<value optimized out>, __dest=<value optimized out>) at hashmgr.cpp:191 #7 0x00007ffff1538313 in HashMgr::load_tables (this=0x324a430, tpath=<value optimized out>, key=<value optimized out>) at hashmgr.cpp:527 #8 0x00007ffff1538473 in HashMgr::HashMgr (this=0x324a430, tpath=0x7fffffff9360 "/usr/lib64/xulrunner-1.9.1/dictionaries/en_ZW.dic", apath=<value optimized out>, key=0x0) at hashmgr.cpp:105 #9 0x00007ffff153cae5 in Hunspell::Hunspell (this=0x324a340, affpath=0x3912d38 "/usr/lib64/xulrunner-1.9.1/dictionaries/en_ZW.aff", dpath=0x7fffffff9360 "/usr/lib64/xulrunner-1.9.1/dictionaries/en_ZW.dic", key=0x0) at hunspell.cpp:87 #10 0x00007ffff1527ef2 in mozHunspell::SetDictionary (this=0x39d11d0, aDictionary=<value optimized out>) at mozHunspell.cpp:157 #11 0x00007ffff151e737 in mozSpellChecker::SetCurrentDictionary (this=0x324a2b0, aDictionary=@0x7fffffff9470) at mozSpellChecker.cpp:373 #12 0x00007ffff140afef in nsEditorSpellCheck::SetCurrentDictionary ( this=<value optimized out>, aDictionary=<value optimized out>) at nsEditorSpellCheck.cpp:455 #13 0x00007ffff140c0e0 in nsEditorSpellCheck::InitSpellChecker (this=0x39d0e20, aEditor=0x7ffff0dbeb3c, aEnableSelectionChecking=0) at nsEditorSpellCheck.cpp:212 #14 0x00007ffff1522fba in mozInlineSpellChecker::SetEnableRealTimeSpell (this=0x39cd190, aEnabled=<value optimized out>) at mozInlineSpellChecker.cpp:726 #15 0x00007ffff116072e in nsEditor::SyncRealTimeSpell (this=0x2dcc390) at nsEditor.cpp:1341 #16 0x00007ffff115976b in nsEditor::PostCreate (this=0x228e) at nsEditor.cpp:246 ..... struct hentry { unsigned char blen; // word length in bytes unsigned char clen; // word length in characters (different for UTF-8 enc.) short alen; // length of affix flag vector unsigned short * astr; // affix flag vector struct hentry * next; // next word with same hash code struct hentry * next_homonym; // next homonym word (with same hash code) char var; // variable fields (only for special pronounciation yet) char word; // variable-length word (8-bit or UTF-8 encoding) }; ... // variable-length hash record with word and optional fields struct hentry* hp = (struct hentry *) malloc (sizeof(struct hentry) + wbl + descl); if (!hp) return 1; char * hpw = &(hp->word); strcpy(hpw, word); That's intentionally not allowed with -D_FORTIFY_SOURCE=2, which doesn't allow crossing field boundaries for str*/stp* etc. functions (and still allows that for mem* etc.). Change that to char word[1];, or char word[];, or char word[0]. In the latter two cases (flexible array member and zero length) you obviously need to check all the allocations, because sizeof(struct hentry) will decrease by 1, in the first case not. Only arrays as last fields are flexible array members or handled as poor alternatives to flexible array members by gcc. I've cloned parts of this bug as hunspell bug 507829. The private xulrunner/mozilla copy is mostly identical to hunspell 1.2.8. *** Bug 507558 has been marked as a duplicate of this bug. *** Created attachment 349408 [details]
mozilla hunspell patch
This is Caolan McNamara's hunspell patch applied and rediffed over the private hunspell copy in mozilla/xulrunner. It also applies to mozilla trunk.
Or just +ac_add_options --enable-system-hunspell
Both cure that last fortify fail.
Is the patch documented somewhere? Or rather, is it a part of any hunspell update? We need to propagate the patch to mozilla... See the hunspell bug. It was patched and built in koji yesterday, its in todays rawhide. Caolan filled http://sourceforge.net/tracker/?func=detail&aid=2812045&group_id=143754&atid=756397 Filled upstream bug https://bugzilla.mozilla.org/show_bug.cgi?id=500607 Fixed for me now. Closing as UPSTREAM. *** Bug 508773 has been marked as a duplicate of this bug. *** *** Bug 508582 has been marked as a duplicate of this bug. *** Seems stable again here as well. |