744816 - Fennec crashes in concurrent access of password db

Status: VERIFIED FIXED

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

Description

[Tracy Walker [:tracy]]

Attachment: 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.

Comment 1

[Richard Newman [:rnewman]]

Related to Bug 730495, Bug 741836?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

The causes for those (by our current understanding) require it to be simultaneous with Gecko launching. That is not the case here?

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Looks like an invalid free in GeckoJNI code. I blame wesj.

Comment 4

[Richard Newman [:rnewman]]

(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.

Comment 5

[Wesley Johnston (:wesj)]

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.

Comment 6

[Richard Newman [:rnewman]]

(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…

Comment 7

[Richard Newman [:rnewman]]

I have temporarily disabled the password sync stage until this bug is resolved.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfe59782089

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Bandaid patch

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).

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Fix

I knew this was something stupid staring me in the face.

Comment 10

[Richard Newman [: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>

Comment 11

[Wesley Johnston (:wesj)]

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...

Comment 12

[Richard Newman [:rnewman]]

https://gist.github.com/2401574

might be relevant to this, too.

Comment 13

[Aaron Train [:aaronmt]]

Attachment: Nightly (04/16) - raw log

Running into a somewhat if not exact same crash -- see attached log 

-- 
Nightly (04/16), Galaxy Nexus (Android 4.0.4)

Comment 14

[Richard Newman [:rnewman]]

Setting target milestone for inbound.

Comment 15

[Richard Newman [:rnewman]]

(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 <[e-mail]>
date:        Fri Apr 13 19:41:40 2012 -0700
summary:     Bug 739629 - Expose safe account creation API for profile migration. r=rnewman

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ef563bc49ec2

Comment 18

[Tracy Walker [:tracy]]

verified with m-c nightly build of 20120418