489589 - Creating metaData table fails with SQLITE_LOCKED in sdb_PutMetaData (password manager broken in Fennec)

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in testing wince, I found that when entering a username/password on a site like gmail or facebook, I don't get the popup to save the credentials as I do on Maemo.

This is with blassey's private build from yesterday (Apr 21).

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Login Manager is failing to initialize the storage component. Debug output:

Login Manager: Counting logins matching host: http://twitter.com, formSubmitURL: ,
Login Manager: No alternate nsILoginManagerStorage registered
PwMgr mozStorage: Initializing key3.db with default blank password.
Login Manager: Initialization of storage component failed: [Exception..."Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPK11Token.initPassword]"
nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///Program%20Files/Fennec/xulrunner/components/storage-mozStorage.js :: anonymous :: line 230" data: no]
[JavaScript Error: "this._storage is null" {file: "file:///Program%20Files/Fennec/xulrunner/components/nsLoginManager.js" line: 537}]

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Fails on this line:
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/softoken/sdb.c#1433

sqlite3_prepare_v2 returns SQLITE_ERROR(1)

Stack trace:

	softokn3.dll!sdb_GetMetaData(SDBStr* sdb = 0x633a8a10, const char* id = 0x01153a44, SECItemStr* item1 = 0x37bc7348, SECItemStr* item2 = 0x37bc7228) Line: 1433, Byte Offsets: 0x80	C
 	softokn3.dll!sftkdb_ChangePassword(SFTKDBHandleStr* keydb = 0x607d6900, char* oldPin = 0x00000000, char* newPin = 0x37bc74cc, int* tokenRemoved = 0x37bc74bc) Line: 1209, Byte Offsets: 0x11c	C
 	softokn3.dll!NSC_InitPIN(unsigned long int hSession = 16777223, unsigned char* pPin = 0x37bc765c, unsigned long int ulPinLen = 0) Line: 3256, Byte Offsets: 0x190	C
 	nss3.dll!PK11_InitPin(PK11SlotInfoStr* slot = 0x607c9400, const char* ssopw = 0x7b9e369e, const char* userpw = 0x37bc765c) Line: 449, Byte Offsets: 0x1c0	C
 	xul.dll!nsPK11Token::InitPassword(const unsigned short* initialPassword = 0x5fdc9100) Line: 348, Byte Offsets: 0x90	C++
 	asmXPTC_InvokeByIndex

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

The SQL used is:

"SELECT ALL * FROM metaData WHERE id=$ID;"

But I don't see a "metaData" table in the DB. Only "nssPrivate" is visible for me.

Comment 4

[Kouhei Sutou]

It's a problem of sdb_PutMetaData() not sdb_GetMetaData().
sdb_PutMetaData() should create metaData table if it's not existed. But it fails to create metaData because SQLite database is busy.

I'll look into it.

Comment 5

[Kouhei Sutou]

Attachment: creates metaData table on initialize.

The patch creates metaData table on initialize instead of on-demand metaData creation. I don't know why but "CREATE TABLE metaData ..." SQL between sdb_Begin() and sdb_Commit() is failed with SQLITE_LOCKED.

If there is any strong reason to do on-demand metaData table creation, the patch will be a solution. ("CREATE TABLE metaData ..." SQL will be needed after "DROP TABLE IF EXISTS metaData" SQL in sdb_Reset() but the patch doesn't include it. I want to confirm whether this idea is valid or not before adding the change.)

Comment 6

[Robert Relyea]

The lack of a metaData table is actually an indication of an uninitialized token, and an important indicator. We don't want to create it when we start.

The more important question, however, is why is the database locked at this point. It seems creating the table before hand will not solve the issue that the database is locked. Our insert won't work any better than a table create in that case..

bob

Comment 7

[Kouhei Sutou]

"INSERT" is worked if metaData table already exists.
It seems that database schema modification is only locked.
ref. http://www.sqlite.org/sharedcache.html - "2.3 Schema (sqlite_master) Level Locking"

I traced "CREATE TABLE metaData ..." and noticed it tried to lock sqlite_master with write-mode. But the locking is failed and SQLITE_LOCKED is returned. Maybe "BEGIN IMMEDIATE TRANSACTION" also locks sqlite_master with write-mode but I don't confirm it. Sorry.


BTW, where is the "uninitialized token indicator" used? I grep-ed mozilla-central/security/nss/lib/ but I couldn't find a code that uses the indicator.
(I assumed that "inUpdate" in sdb_init() arguments is the indicator. Right?)

Comment 8

[Kouhei Sutou]

Attachment: close read DB when write DB is locked.

I noticed that read DB has write-lock when write DB is locked. This problem can be fixed by closing read DB before "CREATE TABLE" and reopening read DB after "CREATE TABLE".

Comment 9

[Doug Turner (:dougt)]

shawn, nelson, does this look right?

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> "INSERT" is worked if metaData table already exists.
> It seems that database schema modification is only locked.
> ref. http://www.sqlite.org/sharedcache.html - "2.3 Schema (sqlite_master) Level
> Locking"
Is NSS acutally using the shared cache?  It doesn't make sense to do so unless you use more than one connection on the same DB file for what it's worth.

(In reply to comment #8)
> I noticed that read DB has write-lock when write DB is locked. This problem can
> be fixed by closing read DB before "CREATE TABLE" and reopening read DB after
> "CREATE TABLE".
So, how is that read DB getting the write lock?  It will only do that if you execute a write statement, otherwise it will obtain a shared lock.  The write DB should be getting pending or exclusive locks (maybe reserved if you open up a transaction that way).

Opening and closing a DB isn't exactly trivial, so we should try to figure out what the actual issue is here.

Comment 11

[Robert Relyea]

> I noticed that read DB has write-lock when write DB is locked. This problem can
> be fixed by closing read DB before "CREATE TABLE" and reopening read DB after
> "CREATE TABLE".

It get's reflect as in PKCS #11 as no password entry. I think the actual table is also used in the case of update. If we can't the the password for the old database, then we don't create the tables.

> So, how is that read DB getting the write lock?

And why is it WIN CE that is getting the write lock and no other platforms?

bob

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

> And why is it WIN CE that is getting the write lock and no other platforms?

I suspect that the difference is not WinCE, but rather that Fennec/PSM is 
doing something different than Firefox/PSM.  

Fennec Guys,
Is is possible to build Fennec on Windows XP or Vista, not as a cross-compiled
app for a PDA, but as a native Windows desktop app?  If so, then doing so might
help us figure out what's different.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

building for native win32 is pretty straightforward:
https://wiki.mozilla.org/Mobile/Build/Fennec

It is just a Firefox build with the mobile-browser repository and a different .mozconfig.

For fennec, this works (or used to) on the maemo platform.  That doesn't mean it is just a wince problem, but it is strong evidence for that.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

We have also been releasing desktop builds along with our device builds.  You can find the Win32 build here: http://ftp.mozilla.org/pub/mozilla.org/mobile/1.0b1/fennec-1.0b1.en-US.win32.zip.

I have to admit though that there are probably some differences in the configure options other than the target.  If what's already posted doesn't get you where you need to go, please ping me again and I'll post a Win32 build with similar build options.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #14)

> I have to admit though that there are probably some differences in the
> configure options other than the target.  

Yes, Win32 build can not be built with --disable-dbm, since the option breaks Win32 build. See bug bug 493364.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Same error on WinXP with --disable-dbm build:

avaScript error: file:///c:/cygwin/home/user/hg/mozilla-central/objdir-winxp-db
g/mobile/dist/bin/xulrunner/components/nsLoginManager.js, line 544: this._storag
e is null
--DOMWINDOW == 11 (050CD778) [serial = 11] [outer = 03CC5878] [url = https://bug
zilla.mozilla.org/]

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Same error on Firefox --disable-dbm on WinXP.

JavaScript error: file:///c:/cygwin/home/user/hg/mozilla-central/objdir-firefox/
dist/bin/components/nsLoginManager.js, line 544: this._storage is null

Comment 18

[Kouhei Sutou]

I also confirmed that this problem is reproduced with --disable-dbm build Fennec on Linux.

If you can find ~/.mozilla/fennec/*.default/key4.db, this problem will be reproduced. If key3.db is found, this problem will not be reproduced because sdb.c isn't used.

Comment 19

[Kouhei Sutou]

(In reply to comment #10)
> (In reply to comment #7)
> > "INSERT" is worked if metaData table already exists.
> > It seems that database schema modification is only locked.
> > ref. http://www.sqlite.org/sharedcache.html - "2.3 Schema (sqlite_master) Level
> > Locking"
> Is NSS acutally using the shared cache?  It doesn't make sense to do so unless
> you use more than one connection on the same DB file for what it's worth.

sqlite3_enable_shared_cache(1) is called at storage/src/mozStorageService.cpp. And mozStorageService.cpp is used before sdb.c. So SQLite3's global "shared cache enable configuration" is used in sdb.c implicitly.

I confirmed that this problem isn't reproduced when calling sqlite3_enable_shared_cache(0) before sdb_OpenDB().

> (In reply to comment #8)
> > I noticed that read DB has write-lock when write DB is locked. This problem can
> > be fixed by closing read DB before "CREATE TABLE" and reopening read DB after
> > "CREATE TABLE".
> So, how is that read DB getting the write lock?  It will only do that if you
> execute a write statement, otherwise it will obtain a shared lock.  The write
> DB should be getting pending or exclusive locks (maybe reserved if you open up
> a transaction that way).

I don't find the reason yet. Sorry.

Comment 20

[Shawn Wilsher :sdwilsh]

It would not be ideal for NSS to be calling that function without holding the lock that we hold when we change that...

I can see that maybe storage shouldn't be automatically turning on the shared cache though...

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 19,
So, this is a problem seen only in the browser, not in NSS's other test 
programs.  I wonder what other sorts of stateful interactions occur.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix

Begin transaction just before sdb_PutMetaData.

Actually I do not understand why this fix resolved the issue but this fix works both on WinXP and WinMo.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Out of curiosity, The db for sdb_GetMetaData is obtained by SFTK_GET_SDB and other dbs (for sdb_PutMetaData or sdb_Commit) are keydb->db itself. Is this intended?

And moreover, sftkdb_CheckPassword and sftkdb_HasPasswordSet use sftk_getPWSDB to get db handle, but sftkdb_ChangePassword uses SFTK_GET_SDB. Is this also intended?

Comment 24

[Robert Relyea]

Comment on attachment 378789 [details] [diff] [review]
Fix

r- for a couple of reasons.

1) We do the begin before we GetMetaData because we need to make sure the password doesn't change out from underneath us need.

2) we need to have the transaction open while we convert the keys in the database. 

I suspect this patch will fail to change the password on any database that has actual keys and certs in them.


One possible solution (hack) that might work is to call sdb_abort in else side of the if (crv == CKR_OK) where crv is the return code from sdb_GetMetaData., then begin a new transaction.

If that logic works, it will get an r+.

bob

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: An attempt that Robert suggested in comment #24

Unfortunately this patch does not resolve the issue.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #24)
> 
> I suspect this patch will fail to change the password on any database that has
> actual keys and certs in them.

Attachment 378789 [details] [diff] can change the password in database. The steps I confirmed are:

1. clear all Fennec profile data
2. start Fennec
3. open bugzilla.mozilla.org
4. enter my E-mail address and *invalid* password
5. click "Log in"
6. choose "Remember"
7. confirm log-in failure (since the password is wrong)
8. log in again with correct E-mail address and password
9. choose "Change"
10. confirm log-in success
11. log out
12. click "Log in"
13. confirm E-mail address and password are filled automatically in form
14. clock "Log in"
15. success!

Comment 27

[Robert Relyea]

I'm not sure what those steps are supposed to show. They appear to be changing a web password (bugzilla.mozilla.org).

It's possible to use SDR to show the 'database password' can be changed, but it's much clearer if you actually use a private key.

Try...
1. clear all profile data.
2. Get a user certificate (http://www.startssl.com or http://www.thawte.com) both sites have free email verified certificates. (in the process it should set up a master password.
3. got to preference->advanced->View Certificates->your Certificates to make sure you have a certificate.
4. Select your certificate and click 'backup' since I suspect the next step will end up being unrecoverable destructive on your system (follow the backup dialogs as the pop up).
5.Close the certificate manager and in the security manager select 'Security->Change Master Password'. Change your password to something else... At this point you should fail (though we see failures don't always get to the user.
6. Go back to the certificate Manager and try to back-up your certs with your new password. This step will almost certainly fail.

bob

NOTE: Even if it doesn't, The Begin is an important concurrency primitive. If more than one app tries to change the password at the same time, the begin/Commit/Abort code will guarantee that one of the passwords will be set correctly. Without such code, you may wind up with a database that has keys encrypted with one password and the password record encrypted with another, effectively lossing access to the keys.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

I understand what you are saying now. Thank you for your clarification.

Comment 29

[Robert Relyea]

BTW, I appreciate your coming up with these patches. Even though they may not be the final, they certainly point out what may be going wrong.

bob

Comment 30

[Hiroyuki Ikezoe (:hiro)]

I confirmed the latest CVS sqlite3 does not cause this issue. So I think this issue is a problem of sqlite3 itself. 

I will investigate what change in sqlite3 solved this issue from now on.
Now I suspect changes in queryTableLock solved the issue.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

I captured it!

The change set is exactly http://www.sqlite.org/cvstrac/chngview?cn=6440

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Attachment: The fix for mozilla-central

I do not know how to take actions like this case, anyway I attach the patch adopted to mozilla-central.

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Oops! I accidentally removed the dependency.

Comment 34

[Shawn Wilsher :sdwilsh]

So, we don't actually take patches to SQLite in mozilla-central, but this will get fixed by bug 493560.

This feels like it should block too since it breaks the password manager.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

... for Fennec on Windows Mobile, which won't block the Firefox 3.5 release. Please renominate if I've misinterpreted the information in this bug.

Comment 36

[Shawn Wilsher :sdwilsh]

Fixed with bug 493560.