494603 - Update NSS's copy of sqlite3 to 3.6.22 to get numerous bug fixes

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

Bug 489589 and bug 493560 show numerous problems in sqlite3 that have 
been fixed with a later version than the copy we have in NSS.  
Mozilla-central is taking version 3.6.14.1 to resolve these issues 
(plus a couple of additional patches, see bug 493560).  
IMO, we should do the same for NSS's copy.

Comment 1

[Wan-Teh Chang]

I will work on this.  The current recommended version of SQLite
is version 3.6.21.

Comment 2

[Wan-Teh Chang]

Attachment: Add the sqlite directory to DIRS in a different way (checked in)

This patch is a "cosmetic" change, but it allows me to see
in one place (manifest.mn) what directories are listed in
the DIRS variable.

I'm using the same method to add the zlib directory to DIRS
optionally.

Comment 3

[Wan-Teh Chang]

Attachment: Supporting changes for updating sqlite3.{h,c} from 3.3.17 to 3.6.21

I dropped in sqlite3.{h,c} from sqlite-amalgamation-3.6.21.tar.gz.
Unfortunately the patch for those two files exceeds the maximum
size of attachments for Bugzilla, so I can't attach it here.

The supporting changes are in this patch.  They include:
- Building libsqlite3.dylib with the same compatibility and current
  version numbers as those of the system libsqlite3.dylib.  It turns
  out these version numbers come from sqlite upstream, not Apple.
- Remove sqlite3_apis (a global variable that was removed) and
  add sqlite3_open_v2 (a new function referenced by Mac's Security
  Framework).  It's too time consuming to figure out and add all
  the new functions, so I only added the one that caused sign.sh
  to crash on Snow Leopard.

Comment 4

[Wan-Teh Chang]

Bob, I verified that the changes you made to our sqlite3.c in
rev. 1.3 and rev. 1.4 are already in sqlite 3.6.21.

I wonder if we should compile with -DSQLITE_THREADSAFE=1.
If I configure sqlite upstream with the --enable-threadsafe
option, it compiles with -DSQLITE_THREADSAFE=1.  If I don't
specify any configure option, it compiles with
-DSQLITE_THREADSAFE=0.

Comment 5

[Wan-Teh Chang]

Attachment: Supporting changes for updating sqlite3.{h,c} from 3.3.17 to 3.6.21, v1.1 (checked in)

I improved the comments.  No real change.

Comment 6

[Robert Relyea]

Comment on attachment 418179 [details] [diff] [review]
Supporting changes for updating sqlite3.{h,c} from 3.3.17 to 3.6.21, v1.1 (checked in)

r+ rrelyea

Comment 7

[Robert Relyea]

re: -DSQLITE_THREADSAFE=1

I'm ok with turning that on. It is on in the system version for Linux. as long as it doesn't break any of our build platforms, that should be fine.

It shouldn't affect mozilla since they have their own copy of sqlite.

Comment 8

[Wan-Teh Chang]

Comment on attachment 417731 [details] [diff] [review]
Add the sqlite directory to DIRS in a different way (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6).

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/Makefile,v  <--  Makefile
new revision: 1.14; previous revision: 1.13
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.21; previous revision: 1.20
done

Comment 9

[Wan-Teh Chang]

Comment on attachment 418179 [details] [diff] [review]
Supporting changes for updating sqlite3.{h,c} from 3.3.17 to 3.6.21, v1.1 (checked in)

SQLite 3.6.22 was just released yesterday (2010-01-06), so I
upgraded to 3.6.22 instead.

I checked in this patch on the NSS trunk (NSS 3.12.6).

Checking in coreconf/Darwin.mk;
/cvsroot/mozilla/security/coreconf/Darwin.mk,v  <--  Darwin.mk
new revision: 1.23; previous revision: 1.22
done
Checking in nss/lib/sqlite/config.mk;
/cvsroot/mozilla/security/nss/lib/sqlite/config.mk,v  <--  config.mk
new revision: 1.7; previous revision: 1.6
done
Checking in nss/lib/sqlite/sqlite.def;
/cvsroot/mozilla/security/nss/lib/sqlite/sqlite.def,v  <--  sqlite.def
new revision: 1.3; previous revision: 1.2
done
Checking in nss/lib/sqlite/sqlite3.c;
/cvsroot/mozilla/security/nss/lib/sqlite/sqlite3.c,v  <--  sqlite3.c
new revision: 1.5; previous revision: 1.4
done
Checking in nss/lib/sqlite/sqlite3.h;
/cvsroot/mozilla/security/nss/lib/sqlite/sqlite3.h,v  <--  sqlite3.h
new revision: 1.3; previous revision: 1.2
done

Comment 10

[Wan-Teh Chang]

Attachment: THREADSAFE has been renamed SQLITE_THREADSAFE (checked in)

It turns out that SQLITE_THREADSAFE is just a renaming of THREADSAFE,
and we are already compiling with -DTHREADSAFE=1.

SQLITE_THREADSAFE=1 is actually the default, so we can also just remove
this line from manifest.mn.  See the following excerpt from sqlite3.c:

/*
** The SQLITE_THREADSAFE macro must be defined as either 0 or 1.
** Older versions of SQLite used an optional THREADSAFE macro.
** We support that for legacy
*/
#if !defined(SQLITE_THREADSAFE)
#if defined(THREADSAFE)
# define SQLITE_THREADSAFE THREADSAFE
#else
# define SQLITE_THREADSAFE 1
#endif
#endif

Comment 11

[Wan-Teh Chang]

Attachment: Fix Solaris gettimeofday compilation error (checked in)

When compiled with -D_SVID_GETTOD, Solaris's gettimeofday takes
only one argument.  See
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/sys/time.h

Since we compile with -D_SVID_GETTOD, we need this patch.  The
previous revision (rev. 1.4) of our sqlite3.c has this change, but
based on the CVS history I am not sure if this is a local change or
comes from sqlite upstream.

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

Comment 12

[Wan-Teh Chang]

Comment on attachment 420688 [details] [diff] [review]
Fix Solaris gettimeofday compilation error (checked in)

I looked at the CVS history again and found that this is our local
change (in rev. 1.1.2.2 on the NSS_BOB_SHARED branch).

----------------------------
revision 1.1.2.2
date: 2007/06/12 22:01:20;  author: rrelyea%redhat.com;  state: Exp;  lines: +41 -37


Handle system 5 gettimeofday.
----------------------------

I verified with the command
  cvs diff -u -r1.1.2.1 -r1.1.2.2 -kk sqlite3.c

(r1.1.2.2 is the same as r1.2.)

We should have a better way to document our local changes.
For this particular case, perhaps we should stop compiling with
-D_SVID_GETTOD.

Comment 13

[Slavomir Katuscak]

Sqlite patch brought new memory leak on Solaris x86 - detected on Tinderbox machine aquash. Leak is detected in PKIX chains tests that uses vfychain tool.

Solaris debug build:

Block in use (biu):
Found block of size 104 bytes at address 0x8089c60 (0.90% of total)
At time of allocation, the call stack was:
        [1] _strdup() at 0xb3d4b603 
        [2] _getcwd() at 0xb3d357d5 
        [3] unixFullPathname() at line 25635 in "sqlite3.c"
        [4] sqlite3OsFullPathname() at line 12167 in "sqlite3.c"
        [5] sqlite3PagerOpen() at line 34381 in "sqlite3.c"
        [6] sqlite3BtreeOpen() at line 39359 in "sqlite3.c"
        [7] sqlite3BtreeFactory() at line 96460 in "sqlite3.c"
        [8] openDatabase() at line 96856 in "sqlite3.c"
        [9] sqlite3_open() at line 96969 in "sqlite3.c"
        [10] sdb_openDB() at line 606 in "sdb.c"
        [11] sdb_init() at line 1713 in "sdb.c"
        [12] s_open() at line 2007 in "sdb.c"
        [13] sftk_DBInit() at line 2595 in "sftkdb.c"
        [14] SFTK_SlotReInit() at line 2156 in "pkcs11.c"
        [15] SFTK_SlotInit() at line 2261 in "pkcs11.c"
        [16] nsc_CommonInitialize() at line 2650 in "pkcs11.c"

Solaris optimized build:

Block in use (biu):
Found block of size 104 bytes at address 0x808ae68 (0.90% of total)
At time of allocation, the call stack was:
        [1] _strdup() at 0xb3d4b603 
        [2] _getcwd() at 0xb3d357d5 
        [3] unixFullPathname() at 0xac512e0c 
        [4] sqlite3PagerOpen() at 0xac5196df 
        [5] sqlite3BtreeOpen() at 0xac51e085 
        [6] sqlite3BtreeFactory() at 0xac5a0297 
        [7] openDatabase() at 0xac5a0b2e 
        [8] sqlite3_open() at 0xac5a0cdb 
        [9] sdb_init() at 0xad92d4d3 
        [10] s_open() at 0xad92e38c 
        [11] sftk_DBInit() at 0xad932945 
        [12] SFTK_SlotReInit() at 0xad910dfd 
        [13] SFTK_SlotInit() at 0xad9111ae 
        [14] nsc_CommonInitialize() at 0xad9120ee 
        [15] NSC_Initialize() at 0xad9123b7 
        [16] secmod_ModuleInit() at 0xc0541fea

Comment 14

[Wan-Teh Chang]

Slavo, thank you for the stack traces of the leaks.  I was going to
file a bug report and ask you to provide this info.  Are these the
only new leaks?

Since sqlite's internal function names may change in a new
release, could you find out if this is just a variant of a known
leak that we have a filter for?

Comment 15

[Wan-Teh Chang]

Slavo, I looked at the relevant sqlite code.  It's entirely contained
in the unixFullPathname function in sqlite3.c, so it's easy to
inspect.

This is a leak in Solaris's getcwd() function.  If we only get this
leak during NSS initialization, we should just add a filter for
this leak.  Also, this leak may have been fixed in Solaris 10 and
later because we only see it on Solaris 9.

Comment 16

[Shawn Wilsher :sdwilsh]

Do you plan on upstreaming your changes to SQLite?

Comment 17

[Robert Relyea]

Comment on attachment 420659 [details] [diff] [review]
THREADSAFE has been renamed SQLITE_THREADSAFE (checked in)

r+

Comment 18

[Robert Relyea]

Comment on attachment 420688 [details] [diff] [review]
Fix Solaris gettimeofday compilation error (checked in)

r+

Comment 19

[Wan-Teh Chang]

Comment on attachment 420659 [details] [diff] [review]
THREADSAFE has been renamed SQLITE_THREADSAFE (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6).

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

Comment 20

[Wan-Teh Chang]

Attachment: Add README to document sqlite version and local changes (checked in)

Re: comment 6

sdwilsh: I'd rather not compile NSS with -D_SVID_GETTOD.  The
two-argument version of gettimeofday is the standard and
is the default in Solaris.

Unfortunately I can't remove -D_SVID_GETTOD right away
because I also need to change lib/freebl/unix_rand.c, which
is now frozen for FIPS validation.

Comment 21

[Wan-Teh Chang]

That should be Re: comment 16.  Sorry.

Comment 22

[Slavomir Katuscak]

(In reply to comment #15)
> Slavo, I looked at the relevant sqlite code.  It's entirely contained
> in the unixFullPathname function in sqlite3.c, so it's easy to
> inspect.
> 
> This is a leak in Solaris's getcwd() function.  If we only get this
> leak during NSS initialization, we should just add a filter for
> this leak.  Also, this leak may have been fixed in Solaris 10 and
> later because we only see it on Solaris 9.

Right, it seems that this is the case of bug 463208, but stacks have changed. I just updated list of ignored leaks with new pattern **/unixFullPathname/_getcwd/**.

Comment 23

[Robert Relyea]

Comment on attachment 420864 [details] [diff] [review]
Add README to document sqlite version and local changes (checked in)

r+ rrelyea

Comment 24

[Wan-Teh Chang]

Comment on attachment 420864 [details] [diff] [review]
Add README to document sqlite version and local changes (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.6).

RCS file: /cvsroot/mozilla/security/nss/lib/sqlite/README,v
done
Checking in README;
/cvsroot/mozilla/security/nss/lib/sqlite/README,v  <--  README
initial revision: 1.1
done