With the latest SDK based last pass add-on (https://rodan.lastpass.com/dev/lp_e10s.xpi) I get this crash all the time: * thread #1: tid = 0x25761, 0x0000000101dde235 XUL`nsTArray_Impl<unsigned int, nsTArrayInfallibleAllocator>::ElementAt(this=0x000000012ffef488, aIndex=1) + 117 at nsTArray.h:984, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x0000000101dde235 XUL`nsTArray_Impl<unsigned int, nsTArrayInfallibleAllocator>::ElementAt(this=0x000000012ffef488, aIndex=1) + 117 at nsTArray.h:984 frame #1: 0x0000000101dcb06d XUL`nsTArray_Impl<unsigned int, nsTArrayInfallibleAllocator>::operator[](this=0x000000012ffef488, aIndex=1) + 29 at nsTArray.h:1019 frame #2: 0x0000000106e421c1 XUL`nsAutoCompleteController::ProcessResult(this=0x000000012ffef440, aSearchIndex=1, aResult=0x0000000128c4ad40) + 417 at nsAutoCompleteController.cpp:1544 frame #3: 0x0000000106e41ff6 XUL`nsAutoCompleteController::HandleSearchResult(this=0x000000012ffef440, aSearch=0x000000012b31bd20, aResult=0x0000000128c4ad40) + 118 at nsAutoCompleteController.cpp:781 frame #4: 0x0000000106e42803 XUL`nsAutoCompleteController::OnSearchResult(this=0x000000012ffef440, aSearch=0x000000012b31bd20, aResult=0x0000000128c4ad40) + 307 at nsAutoCompleteController.cpp:823 frame #5: 0x0000000106e429d7 XUL`non-virtual thunk to nsAutoCompleteController::OnSearchResult(this=0x000000012ffef440, aSearch=0x000000012b31bd20, aResult=0x0000000128c4ad40) + 55 at nsAutoCompleteController.cpp:801 frame #6: 0x0000000101f69cae XUL`::NS_InvokeByIndex(that=0x000000012ffef440, methodIndex=3, paramCount=2, params=0x00007fff5fbf5c60) + 558 at xptcinvoke_x86_64_unix.cpp:174 frame #7: 0x0000000102f9e6b8 XUL`CallMethodHelper::Invoke(this=0x00007fff5fbf5c18) + 88 at XPCWrappedNative.cpp:2097 frame #8: 0x0000000102f95691 XUL`CallMethodHelper::Call(this=0x00007fff5fbf5c18) + 257 at XPCWrappedNative.cpp:1414 frame #9: 0x0000000102f73f4c XUL`XPCWrappedNative::CallMethod(ccx=0x00007fff5fbf5dc8, mode=CALL_METHOD) + 252 at XPCWrappedNative.cpp:1381 frame #10: 0x0000000102f76027 XUL`XPC_WN_CallMethod(cx=0x0000000100384000, argc=2, vp=0x000000011cfd51c0) + 807 at XPCWrappedNativeJSOps.cpp:1126 frame #11: 0x000000010877eb8d XUL`js::CallJSNative(cx=0x0000000100384000, native=(XUL`XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) at XPCWrappedNativeJSOps.cpp:1105), args=0x00007fff5fbf7f78)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 173 at jscntxtinlines.h:235 frame #12: 0x000000010873d19b XUL`js::Invoke(cx=0x0000000100384000, args=0x00007fff5fbf7f78, construct=NO_CONSTRUCT) + 1163 at Interpreter.cpp:489 frame #13: 0x00000001087560fa XUL`Interpret(cx=0x0000000100384000, state=0x00007fff5fbf9020) + 55914 at Interpreter.cpp:2798 frame #14: 0x0000000108748595 XUL`js::RunScript(cx=0x0000000100384000, state=0x00007fff5fbf9020) + 725 at Interpreter.cpp:430 frame #15: 0x000000010873d270 XUL`js::Invoke(cx=0x0000000100384000, args=0x00007fff5fbf9158, construct=NO_CONSTRUCT) + 1376 at Interpreter.cpp:507 frame #16: 0x00000001085580e4 XUL`js::fun_call(cx=0x0000000100384000, argc=2, vp=0x00007fff5fbf9418) + 516 at jsfun.cpp:1192 frame #17: 0x000000010877eb8d XUL`js::CallJSNative(cx=0x0000000100384000, native=(XUL`js::fun_call(JSContext*, unsigned int, JS::Value*) at jsfun.cpp:1174), args=0x00007fff5fbf93c0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 173 at jscntxtinlines.h:235 frame #18: 0x000000010873d19b XUL`js::Invoke(cx=0x0000000100384000, args=0x00007fff5fbf93c0, construct=NO_CONSTRUCT) + 1163 at Interpreter.cpp:489 frame #19: 0x0000000108764167 XUL`js::Invoke(cx=0x0000000100384000, thisv=0x00007fff5fbf9a20, fval=0x00007fff5fbf94e0, argc=2, argv=0x00007fff5fbf9a28, rval=JS::MutableHandleValue @ 0x00007fff5fbf93b0) + 743 at Interpreter.cpp:542 frame #20: 0x000000010868ea2e XUL`js::DirectProxyHandler::call(this=0x000000010bf346f0, cx=0x0000000100384000, proxy=JS::HandleObject @ 0x00007fff5fbf9508, args=0x00007fff5fbf9760) const + 254 at DirectProxyHandler.cpp:77 frame #21: 0x000000010867fbc9 XUL`js::CrossCompartmentWrapper::call(this=0x000000010bf346f0, cx=0x0000000100384000, wrapper=JS::HandleObject @ 0x00007fff5fbf9610, args=0x00007fff5fbf9760) const + 521 at CrossCompartmentWrapper.cpp:289 frame #22: 0x0000000108692e46 XUL`js::Proxy::call(cx=0x0000000100384000, proxy=JS::HandleObject @ 0x00007fff5fbf9700, args=0x00007fff5fbf9760) + 390 at Proxy.cpp:412 frame #23: 0x0000000108694673 XUL`js::proxy_Call(cx=0x0000000100384000, argc=2, vp=0x00007fff5fbf9a18) + 211 at Proxy.cpp:710 frame #24: 0x000000010877eb8d XUL`js::CallJSNative(cx=0x0000000100384000, native=(XUL`js::proxy_Call(JSContext*, unsigned int, JS::Value*) at Proxy.cpp:706), args=0x00007fff5fbf99c0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 173 at jscntxtinlines.h:235 frame #25: 0x000000010873d060 XUL`js::Invoke(cx=0x0000000100384000, args=0x00007fff5fbf99c0, construct=NO_CONSTRUCT) + 848 at Interpreter.cpp:477 frame #26: 0x0000000108764167 XUL`js::Invoke(cx=0x0000000100384000, thisv=0x00007fff5fbf9cf0, fval=0x00007fff5fbf9d08, argc=2, argv=0x00007fff5fbf9f20, rval=JS::MutableHandleValue @ 0x00007fff5fbf99b0) + 743 at Interpreter.cpp:542 frame #27: 0x0000000108b58b21 XUL`js::jit::DoCallFallback(cx=0x0000000100384000, frame=0x00007fff5fbf9f88, stub_=0x000000011f58b158, argc=2, vp=0x00007fff5fbf9f10, res=JS::MutableHandleValue @ 0x00007fff5fbf9db8) + 2449 at BaselineIC.cpp:9025 frame #28: 0x0000000115fec00b I'm not sure what is going on but it's pretty clear that we are not guarding the index before accessing the array [1]. Since we're only reading from the address this should not be too dangerous security wise but the next line concerns me... I don't know this code well enough and there is a chance that there is a sec crit bug as well here on the next line (I don't see why it's obvious that resultIndex is not out of bound as well), I'll mark it as a sec bug for now. [1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1544

Tim, I've seen you hacking around nsAutoCompleteController, are you familiar with this code or do you know who owns it?

Pretty sure Marco should know more here. Also, EXC_BAD_ACCESS (code=1, address=0x0) looks like a nullpointer access, which is safe and shouldn't need to be sec-sensitive. Do you have a normal/public crash report for this crash as well?

(In reply to :Gijs Kruitbosch from comment #2) > looks like a nullpointer access, which is safe and shouldn't need to be > sec-sensitive. My concerns is unrelated to this crash. I just don't see why is it safe not to check if resultIndex is within the bounds before calling |mMatchCounts[resultIndex] = matchCount;| in the next line in general. It's not obvious to me. > > Do you have a normal/public crash report for this crash as well? I'm not sure what you mean, crash-stats? Or without the add-on? Then I don't. I've just found this problem mentioned here: https://bugzilla.mozilla.org/show_bug.cgi?id=1008768#c126

I think this is bug 720589, it's there from a long time. I had a patch there but the code is very foggy and trying to cover so many cases it's hard to just do the wrong thing. It's interesting that the add-on can activate it constantly. Likely cause it's messing up with autocomplete for passwords completion. I don't think there's a security risk involved here, plus the bug is known from a long time, so likely we can just dupe this? I'll see if I can find the time to update the patch in the next days.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3) > My concerns is unrelated to this crash. I just don't see why is it safe not > to check if resultIndex is within the bounds before calling > |mMatchCounts[resultIndex] = matchCount;| in the next line in general. It's > not obvious to me. it's not safe, but it's not a security concern, imho. Surely we should fix it.

(In reply to Marco Bonardo [::mak] from comment #4) > it's hard to just do the wrong thing. I guess you figured I meant it's EASY to do the wrong thing.

(In reply to :Gijs Kruitbosch from comment #2) > EXC_BAD_ACCESS (code=1, address=0x0) For the record, that EXC_BAD_ACCESS (code=1, address=0x0) comes from an assert, in release build this is not a nullptr access. mMatchCounts is valid has one element and we are trying to read from index 1. Still a reading though as I stated in Comment 0. (In reply to Marco Bonardo [::mak] from comment #5) > it's not safe, but it's not a security concern, imho. Surely we should fix > it. Just one line above the other index is out of bounds so I see no guarantee that this index cannot be out of bounds too in some cases. In which case we write to some random place which is very much a security concern. You probably have a lot better overview of what are the possible scenarios though and why it cannot happen... > I guess you figured I meant it's EASY to do the wrong thing. That's exactly what worries me. Anyway, we should just fix this, but for the meanwhile I don't want to give ideas to anyone where to look for an easy to exploit buffer overflow error that's why I set security flag on this bug.

I have a local patch that is promising to fix bug 720589, I just need to give it some more thinking.

bug 720589 landed and should be resolved soon. Though I don't think it's safe enough for uplifts, in case we would like to evaluate that, it should ride the full train to release.

Fixed by bug 720589.

Marco: why do you think this is unsafe for uplifts to affected branches? Would we have enough stability data if we skipped this cycle and uplifted for Fx 44?

(In reply to Daniel Veditz [:dveditz] from comment #11) > Marco: why do you think this is unsafe for uplifts to affected branches? > Would we have enough stability data if we skipped this cycle and uplifted > for Fx 44? Because the patch is touching a very commonly hit code path in autocomplete, thus it is worth to give it more testing as possible. I think we may consider it stable enough after a full Nightly cycle, so it may be possible to uplift to 44 early in the Beta cycle.

Fixed as per https://bugzilla.mozilla.org/show_bug.cgi?id=720589#c36

Marco, this is re: https://bugzilla.mozilla.org/show_bug.cgi?id=720589#c39. 1. How complicated is it to re-base that patch to esr38? 2. Given that this bug is a sec-high rated issue, we usually try to uplift the fix to all releases at the same time so we avoid chemspill situations. It is unclear to me, how easy it is to exploit this? IIUC, this is a crash that you say does not occur as frequently in esr38 and rebasing might be too complicated, I am ok with wontfix'ing for esr38. DVeditz, Al: Is it ok to wontfix this for esr38? Please help!

I'd prefer it fixed but if it doesn't make sense to do, we'll survive.

(In reply to Ritu Kothari (:ritu) from comment #14) > 1. How complicated is it to re-base that patch to esr38? I don't know. I don't think code-wise is extremely complex, but the result would be mostly untested cause the underlying code would differ considerably. From a quality point of view I'd not want to ship an invasive change to autocomplete without a full cycle of testing...

(In reply to Marco Bonardo [::mak] from comment #16) > (In reply to Ritu Kothari (:ritu) from comment #14) > > 1. How complicated is it to re-base that patch to esr38? > > I don't know. I don't think code-wise is extremely complex, but the result > would be mostly untested cause the underlying code would differ > considerably. From a quality point of view I'd not want to ship an invasive > change to autocomplete without a full cycle of testing... Thanks Marco. Let's reconsider uplifting this to esr38 in the next cycle (i.e. esr 38.7) if a full cycle of testing helps.

(In reply to Ritu Kothari (:ritu) from comment #17) > Thanks Marco. Let's reconsider uplifting this to esr38 in the next cycle > (i.e. esr 38.7) if a full cycle of testing helps. No, sorry if I was unclear, what I meant is that we'd need a full cycle of testing of the ESR specific patch, that we can't have cause there's nothing like a beta channel for ESR. If the choice is between this bug and an untested (since different) patch with the possibility to break autocomplete and the awesomebar, I'd probably just wontfix esr38 and call it fixed in ESR 45.

Let's won't fix it then.

I'm not able to reproduce any crash with the latest SDK based last pass add-on (https://rodan.lastpass.com/dev/lp_e10s.xpi) with Nightly from 2015-11-05 (under Ubuntu 14.04 32-bit and Mac OS X 10.9.5) and 43.0.4 (under Ubuntu 14.04 32-bit); Marco, can you please provide any guidance in order to reproduce/verify this crash? Thanks in advance!

reproducing requires a very specific setup of an autocomplete binding, that we don't even have in the product at this point (multiple async queries returning multiple results in an unexpected order). I don't know how that specific version of lastpass was causing the crash, I can just guess it was either a misuse of the autocomplete API and they fixed it or it was a specific thread scheduling. Nothing you could just verify without lots of coding and tries I fear. Maybe we could verify this by comparing crash signatures ratio?

Thanks for your prompt reply! In Socorro there are only 2 reports for nsAutoCompleteController::ProcessResult signature, for 38 ESR and 43.0.3 (both with wontfix flag). Although, we'll keep an eye on Socorro and this bug report in the following period, just to be sure everything is safe and sound.