Closed Bug 744816 Opened 8 years ago Closed 8 years ago

Fennec crashes in concurrent access of password db

Categories

(Firefox for Android :: General, defect, P1, blocker)

14 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: tracy, Assigned: wesj)

References

Details

(Keywords: crash, Whiteboard: [native-crash][has reviewed patch])

Attachments

(3 files, 1 obsolete file)

Attached file log of crash
Seen with latest inbound build with password Sync http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android/1334228548/ 

STR's:

0) clean install the given build
1) Setup sync to existing desktop client via jpake
2) attempt to add a password to fennec at say bugzilla.mozilla.org

tested results:
fennec crashes and sync fails

expected results:
no crash, password is added to db and sync succeeds.
blocking-fennec1.0: --- → ?
Keywords: crash
Whiteboard: [native-crash]
The causes for those (by our current understanding) require it to be simultaneous with Gecko launching. That is not the case here?
Looks like an invalid free in GeckoJNI code. I blame wesj.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #3)
> Looks like an invalid free in GeckoJNI code. I blame wesj.

Quick sleuthing! Poor wesj.
Blocks: 709385
Assignee: nobody → wjohnston
blocking-fennec1.0: ? → beta+
Weird. I rewrote encrypt and decrypt to both do nothing but

char* result = (char*)malloc(2048);
free(result);
return jenv->NewStringUTF("");

And I still crash.
(In reply to Wesley Johnston (:wesj) from comment #5)

> And I still crash.

As far as I know, this implies that the problem is more general heap corruption that is simply being detected during free.

Time to audit all of the C++ that runs up until that point…
Blocks: 742104
I have temporarily disabled the password sync stage until this bug is resolved.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfe59782089
Status: NEW → ASSIGNED
Priority: -- → P1
Attached patch Bandaid patch (obsolete) — Splinter Review
I don't like this much, but wanted to have this up as a bandaid patch while I try to figure this out. Basically, just holds a buffer forever.

I can trigger this bug without having to have two threads with concurrent access to the db. It seems related to the length of the string I try to encode (although that also seems unlikely and probably is unrelated).
Attachment #615000 - Flags: feedback?(rnewman)
Attached patch FixSplinter Review
I knew this was something stupid staring me in the face.
Attachment #615000 - Attachment is obsolete: true
Attachment #615000 - Flags: feedback?(rnewman)
Attachment #615004 - Flags: review?(rnewman)
Comment on attachment 615004 [details] [diff] [review]
Fix

Review of attachment 615004 [details] [diff] [review]:
-----------------------------------------------------------------

Heh. Good spot.

Can you write a trivial test that fails when linked with eFence[0] or equivalent? Or file a bug to run cppcheck[1] against this source?

This kind of overrun should be easy to catch automatically, and who knows… there might be more.

[0] <http://en.wikipedia.org/wiki/Electric_Fence>
[1] <http://sourceforge.net/apps/mediawiki/cppcheck/index.php?title=Main_Page>
Attachment #615004 - Flags: review?(rnewman) → review+
Whiteboard: [native-crash] → [native-crash][has reviewed patch]
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef563bc49ec2

I'm looking into using efence (i don't see it used anywhere else in our source, but it seems like it would be a useful thing...). There's also some work using cppcheck (bug 679417), but no bug to automagically run it. I did run it here though. Filing bugs for things it shows...
https://gist.github.com/2401574

might be relevant to this, too.
Running into a somewhat if not exact same crash -- see attached log 

-- 
Nightly (04/16), Galaxy Nexus (Android 4.0.4)
Setting target milestone for inbound.
Target Milestone: --- → Firefox 14
(In reply to Richard Newman [:rnewman] from comment #7)
> I have temporarily disabled the password sync stage until this bug is
> resolved.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfe59782089

This was re-enabled by:

changeset:   91647:190a237b1456
user:        Nick Alexander <nalexander@mozilla.com>
date:        Fri Apr 13 19:41:40 2012 -0700
summary:     Bug 739629 - Expose safe account creation API for profile migration. r=rnewman
Duplicate of this bug: 746133
Blocks: 745039
https://hg.mozilla.org/mozilla-central/rev/ef563bc49ec2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
verified with m-c nightly build of 20120418
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.