217538 - softoken databases cannot be shared between multiple processes

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

The Berkeley DB database code we are using in NSS by default is only safe if one
process is writing to the cert and key databases at a time. If multiple
applications start up, and all initialize NSS read/write, the databases will
become corrupt over time.

Fixing this requires replacing Berkeley DB with a better database engine, which
would talk to a local database server.

Comment 1

[Doncho N. Gunchev]

    rpm uses Berkeley DB too and while only one rpm instance can write at a time
any number of other can read. With the latest rpm from Fedora Core 3 the second
copy requiring write access will also wait first one to finish before writing.
Do mozilla/firefox do this too?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The dbm code now being used in mozilla for the cert and key databases 
safely permits only the following combinations of multi-process access:

1. Multiple processes using a DB, ALL of them using it read-only,
   NONE of them using it read-write, or

2. ONE process using the DB read-write, and NO other processes using it
   read-write or read-only.  

Since mozilla/FF/TB always open it read-write, only one process can 
safely have it open at one time.

Comment 3

[Robert Relyea]

As a further note, because of this restriction mozilla/FF/and TH each have their
own copies of the NSS certificate/key/module databases. This, of course, is not
desirable.

bob

Comment 4

[Wan-Teh Chang]

The Berkeley DB used by NSS (in the source directory mozilla/dbm)
is Berkeley DB 1.85 with our modifications.  This is most likely
older than the Berkeley DB used by rpm.

Comment 5

[Doncho N. Gunchev]

(In reply to comment #4)
> The Berkeley DB used by NSS (in the source directory mozilla/dbm)
> is Berkeley DB 1.85 with our modifications.  This is most likely
> older than the Berkeley DB used by rpm.

Historic, if I get these numbers right:
[root@fedora ~]# rpm -q db4
db4-4.2.52-6

http://www.sleepycat.com/: Berkeley DB 4.3 available now! Download

1.96 is the latest 1.x and is unavailable for download.

From http://www.sleepycat.com/update/2.2.0/if.2.2.html:
| In DB 2.2.0, the handles returned by the DB subsystems, e.g., the db_appinit()
| function's DB_ENV, and the db_open() function's DB, are free-threaded. If you
| specify the DB_THREAD flag to each subsystem, or globally using db_appinit(),
| you can then use the returned handle concurrently in any number of threads.
... but seems incompatible and is not available for download too...

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I believe Bob is planning to work on this in the near future.
Bob, please readjust priority, severity, target as you see fit.

Comment 7

[Robert Relyea]

Patches for the initial landing of the shared database code are forthcoming.

These patches are large, and a number of things are still missing, which I'll document as separate bugs. The design is documented at http://wiki.mozilla.org/NSS_Shared_DB

bob.

Comment 8

[Robert Relyea]

Attachment: Collection 1: core.patch

This is a full copy of the core patch to softoken.

Comment 9

[Robert Relyea]

Attachment: Collection 1a: core.removed.

These are the removed files from the core patch.

Comment 10

[Robert Relyea]

Attachment: Collection 1b: core.diff

Changed files in softoken.

Comment 11

[Robert Relyea]

Attachment: Collection 1c: core.new

New files in sofotken.

Comment 12

[Robert Relyea]

Attachment: Collection 2: legacy db changes.

This the full legacydb directory.

Comment 13

[Robert Relyea]

Attachment: Collection 2a: legacydb.moved.

This is the diff between all the files that were originally in softoken and their new counterparts in legacydb.

Comment 14

[Robert Relyea]

Attachment: Collection 2b: legacydb.extracted

These are editted patches to show the difference between code extracted from files in softoken and their new reflections and independent files in legacydb

Comment 15

[Robert Relyea]

Attachment: Collection 2c: legacydb.extracted.full

This is the same as Collection 2a except the diffs were not editted.

Comment 16

[Robert Relyea]

Attachment: Collection 2d:legacydb.new

These are just the new files for the legacydb.

Comment 17

[Robert Relyea]

Attachment: Collection 3: changes to cmd

Comment 18

[Robert Relyea]

Attachment: Collection 4: changes to test

Comment 19

[Robert Relyea]

Attachment: Explanation of the patches.

This attachement explains the different changes in each of the patches to aid reviews.

Comment 20

[Robert Relyea]

There is one more patch... to add sqlite3. The patch was too large for mozilla since it has all of sqlite3. I'll break the patch up and post it tomorrow.

bob

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Regarding comment 20, and the monster patch for sqlite, 
I think we aren't really going to review all that code.  
Maybe it would suffice to list the new source files, and to 
have a patch that includes only the new Makefile and manifest.mn files
(assuming you have them).

Comment 22

[Robert Relyea]

thanks nelson, I was thinking of something similar, perhaps just headers from the sqlite files.

I pretty much thought the same thing about review it.

bob

Comment 23

[Robert Relyea]

Attachment: Collection 5: changes to add sqlite

This patch includes the build and makefile changes. It includes the headers of the two sqlite3 files. (sqlite3.c and sqlite3.h)

Comment 24

[Neil Williams]

This is not by any means an exhaustive review. I looked mostly at the core patches to see how the legacy DB code works in the new regime. Here are a few questions:

1) sftk_getDBForObject() - maybe sftk_getDBForTokenObject to make the requirement explicit.

2) ftkdb_fixupTemplateIn() - make the ULONG->network byte order code a function;could be optimized on different plaforms?

3) sftkdbLoad_Legacy() - the call through function pointer setCryptFunction, shouldn't that be (*setCryptFunction)(sftkdb_encrypt_stub,sftkdb_decrypt_stub)?

Looks pretty good. It also looks like you've done an awful lot of work, Bob. Wouldn't it be more conservative to keep the legacy code built in and access the new DB through the shared library interface?

Comment 25

[Robert Relyea]

Thanks Neil,

I have no problem changing the name of 'sftk_getDBForObject()' (though Adding token is a bit redundant since session objects never have DB's.

2) May be a macro. You could do the same optimization, but then you would have to detect the endianness of your platform. I believe we work pretty hard not to know that (though I may have reintroduced that knowledge in mpi at one point).

3) your formulation is more clear about what is happenning. I'll correct my  code.

As far as loading the legacy db. I had a couple of reasons for that.
first, there is a lot of code in softoken whose sole purpose is to service the
legacy database. You can see that in the size reduction of softoken itself, and the actual size of lgdbm.
second, all dependencies on libdbm are now pushed to libdbm.
Finally, in the normal operation (the case where we are using the new shared db), you never need to load the legacy db. You could even delete it (platforms that have never stored keys in legacy db's would never need to build or ship lgdbm.

bob

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

A request:  Please change the name "lgdbm" to something else.  
It's unpronouncable, and who knows what it means?  Legacy DBM? 
If that's it, let's call it "LegacyDB".  
We don't need to cram names into 8.3 any more.  

Comment 27

[Robert Relyea]

done, execept I made it all lower case as we have no mixed cased library names in NSS (including things like secutil, etc).

bob

Comment 28

[Julien Pierre]

Nelson, actually we do, shared librariy names on OS/2 have an 8.3 limit in the kernel :( But legacydb fits 8.3 .

Comment 29

[Justin Dolske [:Dolske]]

There's already a sqlite in mozilla/db (which is being updated via bug 341137) -- is there a way avoid having this code being duplicated in two spots? It's rather large.

Comment 30

[glen beasley]

Attachment: build on aix

Comment 31

[Julien Pierre]

OS/2 build failure.

g++  -Zomf -Zdll -Zmap  -o OS22.45_gcc_DBG.OBJ/sqlite3.DLL OS22.45_gcc_DBG.OBJ/s
qlite.def -o OS22.45_gcc_DBG.OBJ/sqlite3.DLL OS22.45_gcc_DBG.OBJ/sqlite3.o

E:\DEV\NSS\tip2\mozilla\security\nss\lib\sqlite\OS22.45_gcc_DBG.OBJ\sqlite3.o(E:
\DEV\NSS\tip2\mozilla\security\nss\lib\sqlite\sqlite3.c) : error LNK2029: "_DosL
oadModule" : unresolved external
E:\DEV\NSS\tip2\mozilla\security\nss\lib\sqlite\OS22.45_gcc_DBG.OBJ\sqlite3.o(E:
\DEV\NSS\tip2\mozilla\security\nss\lib\sqlite\sqlite3.c) : error LNK2029: "_DosF
reeModule" : unresolved external
E:\DEV\NSS\tip2\mozilla\security\nss\lib\sqlite\OS22.45_gcc_DBG.OBJ\sqlite3.o(E:
\DEV\NSS\tip2\mozilla\security\nss\lib\sqlite\sqlite3.c) : error LNK2029: "_DosQ
ueryProcAddr" : unresolved external


There were 3 errors detected
make.exe[2]: Leaving directory `E:/DEV/NSS/tip2/mozilla/security/nss/lib/sqlite'

make.exe[1]: Leaving directory `E:/DEV/NSS/tip2/mozilla/security/nss/lib'

Seems like we need to link one of the OS/2 system libraries, probably os2836.lib .

Comment 32

[Robert Relyea]

Justin RE: extra copy of sqlite.

So NSS is a stand alone component. It needs to build without the rest of mozilla included. There are basically 3 configurations for NSS in this case:

1) built with mozilla - NSS will use the mozilla copy of sqlite3.
2) built for platforms with OS provided sqlite3 (most modern Linux systems, for instance) - NSS will use the system version.
3) All other cases: NSS will use it's own.

Currently sqlite is not built on the tip of mozilla, so NSS will use it's own until sqlite gets turned on in mozilla.

bob

Comment 33

[Robert Relyea]

Comment on attachment 268310 [details] [diff] [review]
build on aix

r= relyea

Exeacly how I envisioned it, Glen has verified that if fixes the AIX build.

Comment 34

[Robert Relyea]

Julian,

It sounds like OS/2 needs a patch similiar to the one glen attached. The file is mozilla/security/nss/lib/sqlite/config.mk

thanks for testing OS/2!

Comment 35

[Robert Relyea]

Attachment: Fix memory leak in new database code.

Comment 36

[Steffen Wilberg]

> Currently sqlite is not built on the tip of mozilla
That's not correct. If I'm not mistaken, all apps build MOZ_STORAGE, and mozila/storage builds mozilla/db/sqlite3/src.
Just look at your profile when using Firefox trunk. I'm counting 6 .sqlite files in there.

Comment 37

[Robert Relyea]

but where is sqlite3.dll? or is the mozilla sqlite linked into some other component library?

Comment 38

[Slavomir Katuscak]

Attachment: Memory leaks - logs.

Bob,

This code brought some memory leaks. I'm attaching logs with details. One-line versions of stacks are in file foundleaks, full stacks in other files. Please check all stack containing functions with lg_ prefix.

Comment 39

[Julien Pierre]

Bob,

Re: comment 32,

I was building NSS standalone, doing gmake nss_build_all . So there was no mozilla copy. And OS/2 doesn't come with sqlite. So we were in the third case.

Re: comment 34,
Yes, I agree. When I have time tonight I will attach a similar patch. That may fix the build of sqlite3.dll . But I expect to run into further issues with legacydb3.dll . I think the OS/2 linker is able to product DLLs with filenames longer than 8.3, but the kernel is not able to load such DLLs. Some optimization went into Warp 3 that removed support for long DLL names (!) and it's never worked since. legacydb3 is 9 characters. So we'll likely need to find something shorter, at least for OS/2. It would be easier if we had a short cross-platform name so the browser products don't have to special-case OS/2 everywhere in their packaging rules.

Comment 40

[Robert Relyea]

Attachment: Rest of the Leaks

These are the rest of the leaks. These are all one-time startup/shutdown, but should be cleaned up when we leave. This patch should do that.

Comment 41

[Robert Relyea]

Julien,

If you need to shorten it, you can do so in the following places:

1) mozilla/security/nss/lib/softoken/legacydb/config.mk - change the shared library name for OS/2.
2) mozilla/security/nss/lib/softoken/lgglue.c - change the name of the library to load.

Options for the name change: 
1) drop the version
2) legacy3
3) legdb3
4) lgdb3

Obviously, I would prefer the shortened name be OS/2 only.

Nelson, is there a similar issue for WINCE?

bob

Comment 42

[Julien Pierre]

Thanks, Bob.

Dropping the version would be OK. Or how about calling it nssdbm3 ?

Comment 43

[glen beasley]

Comment on attachment 268441 [details] [diff] [review]
Rest of the Leaks

the changes all look good.

Comment 44

[Steffen Wilberg]

(In reply to comment #37)
> but where is sqlite3.dll? or is the mozilla sqlite linked into some other
> component library?
It's statically linked into Firefox.exe (or whatever appname) like almost everything else:
http://mxr.mozilla.org/seamonkey/source/db/sqlite3/src/Makefile.in#49

Comment 45

[Robert Relyea]

Comment on attachment 268441 [details] [diff] [review]
Rest of the Leaks

This patch is fine, but it uncovered an existing bug. I'll attach a new patch that fixes both the leaks and the bug soon.

Comment 46

[Robert Relyea]

Attachment: Fix the memory leaks, don't load legacy when using noModDB, make pcertdb shutdown noops if we never startted up

This is pretty much the same patch I obsoleted, except it has a fix to the problem the previous patch uncovered (actually it was 2 problems which colided).

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

It is unacceptable to me to impose 8.3 file names on ALL platforms so that
all platforms can have the same name, even on an old OS that is no longer
produced, and for which commercial software products are no longer being
produced.  If you want to have 8.3 names on OS/2, go ahead, but don't 
restrict any other platforms with that.  

Bob,  WinCE works with long file names too.

Comment 48

[Robert Relyea]

Coolk, then Julien, I'd say make the change OS/2 specific (rather than 8.3 specific). nssdbm3 sounds fine for an OS/2 name.

bob

Comment 49

[Julien Pierre]

Nelson, the 8.3 restriction in OS/2 is specific to DLL filenames, not any other files.

Is there anything wrong with using the name nssdbm3 for all platforms ? I personally prefer it to legacydb3 .

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

I agree.  nssdbm3 is an improvement over legacydb3.  so, go ahead.

Comment 51

[Robert Relyea]

Julien, can you attack a patch for OS/2. I'd like to get alpha nailed today.

Thanks,

bob

Comment 52

[Robert Relyea]

Attachment: rename legacy database to nssdbm

rename legacy database shared library to make OS/2 happy

Comment 53

[Kai Engert [:KaiE:]]

Comment on attachment 268909 [details] [diff] [review]
rename legacy database to nssdbm

r=kengert

Comment 54

[Robert Relyea]

Attachment: turn on shared db in FF 3.0

Comment 55

[Kai Engert [:KaiE:]]

Comment on attachment 268923 [details] [diff] [review]
turn on shared db in FF 3.0

r=kengert

Comment 56

[Stuart Parmenter]

Um, we certainly don't need a second copy of sqlite in the tree.  NSS should be building from mozilla/db instead of importing its own copy.  As for it being statically linked we should look at that, but checking in a second copy and linking to two versions seems like a big no no.  This should have been resolved before this was checked in imho.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

(9:17:42 AM) timeless: you guys realize we intentionally don't want ours to be in global space
(9:17:49 AM) timeless: we're afraid of running into a system version that's incompatible
(9:18:13 AM) timeless: my guess is a pointer table will be the solution
(9:18:30 AM) timeless: so either someone calls SetSQLLitePointers(...) from psm
(9:18:42 AM) timeless: or they don't, and if they don't, nss does loadlibrary(sqllite)
(9:19:20 AM) timeless: kaie: oh, for kicks, iirc we're compiling sqlite w/o threadsafety
(9:19:29 AM) timeless: i hope you guys don't expect it :)
(9:19:35 AM) timeless: (and i'd expect you do...)

FYI

Comment 58

[Robert Relyea]

stuart:
NSS needs to build standalone without Firefox, in fact that is quickly becoming the standard way of getting NSS is to build it by itself. The NSS build of sqlite3 has the ability to turn on or off, and use something else. On some platforms that have sqlite3 built in, we will use the system version. For Mozilla builds, I intend to use the mozilla version, however it's currently statically linked with the FF binary, so that's not going to happen in the alpha. There's a very old standing bug about this (bug 306907) which hasn't gotten much attention by anyone (including me). It's definately something that needs to be looked into for Beta.
Timeless points about threads-safety as well (we can build NSS for non-threadsafe sqlite, but we need to do so at compile time).

Comment 59

[Nelson Bolyard (seldom reads bugmail)]

Emphasizing Bob's point: recall that NSS is used by MANY product, and FireFox
is just one of them.  Most of those products do not build NSS from source
themselves, and instead take the NSS team's standard binary builds of shared
libraries.  Some do build from sources. The NSS team needs to deliver a 
complete set of working NSS shared libs to any of those products that take
binaries.  There also needs to be a single tag with which people can pull
the sources from the mozilla repository and then build and get a working
set of NSS shared libs (incuding SQLite).  NSS doesn't need its own SQLite
sources IFF there is a set of SQLite sources in the tree that NSS can use
as-is to get the SQLite shared library it needs.  If the sources to mozilla's 
current sqlite do not produce a shared lib that may be used with NSS's other 
shared libs, that's a non-starter.  

Finally we DO want a "thread safe" build of SQLite for most products.  
So the sources from which SQLite is built must be able to produce such a
build.  

Since NSS is now capable of building a SQLite shared lib, maybe mozilla 
should start to use that shared lib.  (Just a thought)

Comment 60

[Brendan Eich [:brendan]]

We will not ship two sqlites in Firefox 3. We should ship thread-safe sqlite, it will be needed soon enough if the Google Gears stuff takes off and makes headway in the WHAT-WG. The solution I see is to use a shared, thread-safe sqlite library. It should be built from mozilla/db/sqlite3, and that source should be at a version that all parties agree upon for any given Mozilla product release.

/be

Comment 61

[Julien Pierre]

Bob,

Re: comment 51
I will attach an OS/2 patch tonight to add the linking of the system library. Sorry, I forgot about over the weekend and didn't check my Sun email when I was off.

Comment 62

[Julien Pierre]

Attachment: Proper includes for OS/2 (checked in)

Comment 63

[Julien Pierre]

Comment on attachment 268909 [details] [diff] [review]
rename legacy database to nssdbm

I confirmed that the tree was broken on OS/2 with the "legacydb3.dll" name. The loader was getting an error 123 which is ERROR_INVALID_NAME. The error went undetected in the build even though shlibsign failed. But none of the tools would run after the build due to this error.

After I updated my tree to pick up this patch and rebuilt with nssdbm3.dll, everything was fine.

Comment 64

[Julien Pierre]

Comment on attachment 269057 [details] [diff] [review]
Proper includes for OS/2 (checked in)

Thanks for the review, Bob.

Checking in sqlite3.c;
/cvsroot/mozilla/security/nss/lib/sqlite/sqlite3.c,v  <--  sqlite3.c
new revision: 1.4; previous revision: 1.3
done

Comment 65

[Shawn Wilsher :sdwilsh]

Shouldn't those changes be pushed up into sqlite as well?

Comment 66

[Julien Pierre]

Shawn,

Yes, they should. I wish we only had one source tree for it for sustainability. I hope that issue gets taken care of soon.

I was under the impression sqlite was already part of the browser build on OS/2, so I am not sure why I had to make those changes to build on my OS/2 machine. It could be that the version of the OS/2 toolkit I am using is different than the one the Mozilla browser OS/2 maintainers are using. Regardless, the change I made to sqlite cannot hurt anyone.

Comment 67

[Shawn Wilsher :sdwilsh]

By chance is there a sqlite Ticket number that we can watch for getting them pushed upstream?

Comment 68

[Peter Weilbacher]

Shawn, Julien, I will try to remember to add
# define INCL_DOSMODULEMGR
upstream in SQLite over the weekend. Then it should at least be in 3.4.1. I had never tested the amalgamation version, maybe that's why it always worked for me without that extra #define...

Comment 69

[Peter Weilbacher]

(In reply to comment #68)
> I will try to remember to add # define INCL_DOSMODULEMGR upstream in
> SQLite over the weekend.

Done that, but see Bug 385518. Three more lines will be necessary in the NSS version, too, if compiling/linking with OS/2 high memory support.