302670 - Use the installed libz.so where available

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Mikhail T.]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.4; FreeBSD) KHTML/3.4.1 (like Gecko)
Build Identifier: 

Instead of building its own zlib, NSS should use the Operating System's -lz,  
when possible. The library is part of BSD, Linux, Solaris -- at least.  
  
Similarly, instead of building its own dbm, NSS should use, what's provided by  
-lc on BSD (db-1.85), or libdbm on Linux (SleepyCat's db-4.x). 
 
I'll attach the patches for the FreeBSD port of nss, but you may wish to do 
something similar for other OSes... 

Reproducible: Always

Comment 1

[Mikhail T.]

Attachment: Include <db.h> instead of "mcom_db.h"

Comment 2

[Mikhail T.]

Attachment: Remove dbm and zlib from cmd/platlibs.mk

The zlib hunks will work for all OS-es with -lz.

The dbm hunks expect -lc to contain dbopen() and friends -- true for
BSD-derived systems.

Comment 3

[Julien Pierre]

Does zlib have a stable API ? If we start linking to a system shared library, we
need to ensure that the zlib system libraries will remain binary compatible with
NSS in the future.

Comment 4

[Wan-Teh Chang]

Mikhail:

For dbm, we must use our own copy of Berkeley DB 1.85
because the database format changes between Berkeley
DB 1.85 versions.

For zlib, we can consider using the system zlib when
possible.

Comment 5

[Mikhail T.]

Comment 3 : =Does zlib have a stable API?     
     
Yes.     
     
=If we start linking to a system shared library, we need to ensure that the     
=zlib system libraries will remain binary compatible with NSS in the future.     
     
This is an ABI, rather than an API issue and is best left to the OS vendor (or     
whoever installs the library). If the ABI changes incompatibly, the major     
version of the shared library ought to be bumped. Application vendors should     
not worry about this :-)     
     
Finally, for the paranoid, libz has a (redundant, in my opinion) built-in     
protection against ABI mismatches. At run-time initialization the     
applications's compile-time zlib library version is compared against that     
compiled into the library, that is loaded.     
     
Comment #4 : =because the database format changes between Berkeley DB 1.85 
versions.    
    
Interesting... Are you referring to the endiannes? I thought, this (native vs.    
"network" byte order) can be controled through the db's API.     
    
Even if not in db-1.85, than certainly in the later db-4.x offerings from    
Sleepy Cat (such as shipped with Linux or installable on BSD and elsewhere).    
    
I understand the reluctance to depend on the not-BSD licensed Sleepy Cat    
software, but it can be easily installed on any OS and using it should be an    
option for NSS, even if it is not on by default.    
    
I built nss with on FreeBSD with the patches attached to this "bug" and it    
passed all of the tests, including those in dbtests.sh:    
    
FreeBSD/amd64 results: http://aldan.algebra.com:8015/~mi/nss-blue-results.html    
                       http://aldan.algebra.com:8015/~mi/nss-blue-dbtest.log    
FreeBSD/i386 results:  http://aldan.algebra.com:8015/~mi/nss-mi-results.html   
                       http://aldan.algebra.com:8015/~mi/nss-mi-dbtest.log  
  
Is there anything else I should check, or do the above two test runs mean,    
FreeBSD's db-1.85 implementation is "good enough" to substitute yours?  
 
BTW, the very first of the FIPS tests fails when NSS is compiled against 
NSPR-4.4.1, but passes, when used with NSPR-4.6. Is this a known problem? 

Comment 6

[Wan-Teh Chang]

Comment on attachment 190973 [details] [diff] [review]
Include <db.h> instead of "mcom_db.h"

We can't take this patch even if it works.  We must
use the same Berkeley DB version on all platforms.
Unfortunately we can't upgrade to the latest Sleepycat
Berkeley DB for license reasons.  We have found a
good candidate, SQLite, and are working on the next
generation NSS databases using that.  We will need to
retain the our copy of Berkeley DB 1.85 for a while
so we can upgrade old databases to SQLite.

Comment 7

[Mikhail T.]

Ok, but can we (FreeBSD) use the patch in the mean time? Does the test success 
mean, our db-implementation is compatible with yours? 
 
Seems like it. Comparing NSS' dbm/src code with FreeBSD's, I see no substantial 
differences.  
Both do the swap_header(hashp) when the BYTE_ORDER is LITTLE_ENDIAN.  
  
Here:  
  
     http://www.freebsd.org/cgi/cvsweb.cgi/src/lib/libc/db/hash/ 
 
=Unfortunately we can't upgrade to the latest Sleepycat 
=Berkeley DB for license reasons. 
 
License reasons mean, you can not _ship_ the Sleepycat DB with NSS. But you can 
still use it, if it is present on the system at build-time. Right? 

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

IIRC, we found that if multiple processes are holding a shared sleepycat DB
open, and one of them crashes while in a transaction, the "DB recovery" 
mechanism requires that ALL processes close the DB, then ONE process perform
a "recovery" on the DB, and then all processes can open it again.  

One of the reasons for having the mozilla browser and email components 
separated into separate processes is to allow others to continue when one
fails.  It would not be acceptable to mozilla to make a DB change that 
imposes the requirement that all components must be restarted when one fails.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

In relpy to comment 7, One of the objectives of NSS's DB files is that they
be fully portable from any supported mozilla browser on any platform to 
any other.  That means, a DB made on a big endian 64-bit Unix box must work 
if moved to a little endian 32-bit windows box (for example).  That's why we
don't run different DBs on different boxes.

Comment 10

[Julien Pierre]

Mikhail,

Re: comment #5

We haven't done any testing on FreeBSD, so we don't really "expect" any
particular result for the FIPS test . On other platforms, we have tested NSS
3.10 against NSPR 4.5.x and 4.6, with success . You may want to try 4.5 to
isolate the problem. You should discuss the results in a separate bug.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

This bug is an example of a "compound" bug/request.  It asks for two 
separate changes that are unrelated to each other.  Bugzilla bugs should
each be about just one bug or enhancement request.  

So, I am changing this bug to be only about the use of libz.so on platforms
that have it.  I'd like us to do that in 3.11 if possible.  The sooner we 
can get the place where zlib bugs are not NSS bugs on the major platforms,
the better, IMO.

Another bug could be filed about databases, but we already have a project 
underway to switch to SQLlite, and I think another bug about that subject 
is unnecessary.  

Comment 12

[Mikhail T.]

(In reply to comment #10)  
> You may want to try 4.5 to isolate the problem. 
 
There were no releases between 4.4.1 and 4.6. Hence the bug 276891 . 
 
There was a v4.5.1 "release" on May 20th, but it consisted of Linux binaries 
only (not sure, for which platform even). 
 
  
  

Comment 13

[Mikhail T.]

(In reply to comment #11)  
  
The point of the report was that Mozilla in general and NSS in particular  
should be using pre-installed versions of third-party libraries instead of  
shipping their own.  
  
libz and dbm were just examples :-)  
  
(In reply to comment #9) 
=That means, a DB made on a big endian 64-bit Unix box must work  
=if moved to a little endian 32-bit windows box (for example). 
 
It still seems to me, that FreeBSD's implementation satisfies this requirement. 
Is passing all the tests (all.sh) sufficient to be comfortable? 

Comment 14

[Robert Relyea]

re comment 13:

good point, though in NSS's case there were good reasons for going with a static
dbm (mostly the we were using a version of dbm "frozen" in time because of
license, api, and abi changes, and NSS itself doesn't use zlib (only one of the
NSS commands).

I think this is good input for SQLite, however. SQLite appears to follow a
pretty strict requirements about bumping it's major version number on ABI changes.

bob

Comment 15

[Julien Pierre]

Mikhail,

Re: comment #13,

I don't think passing all.sh is enough . AFAIK all.sh doesn't test any existing
databases written on other platforms. You could replace the entire database
implementation, and it would still pass, as long as the new dbm implementation
is consistent with itself.

Re: comment #12,

There may not have been a binary release or tarball on mozilla.org of NSPR 4.5
and 4.5.1, and this is unfortunate, but the CVS tags tell all about NSPR
releases. These NSPR releases most definitely existed. Solaris 10 shipped with
NSPR 4.5.1 in fact .

I agree with Nelson in comment #11 that this needs to be a separate bug about
using the system dbm . This bug is now about using libz only, and is this enough
for one bug, given all the platform issues . Mikhail, please open a separate bug
for the DB issue .

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Re: comment 12

Not all releases are "binary" releases.  Some are "source" releases.
Most of the products that use NSS and NSPR build NSS and NSPR themselves 
from sources.  They pull the sources using the release's CVS tag, and build 
the binaries themselves.  So, some releases are never released as binaries.  
Any developer who builds NSS himself from source should have no problem 
building NSPR 4.5 from sources.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

The case for using the OS's native libz is much stronger than the case for 
using the OS'es libdbm.  NSS's dbm is part of a FIPS certified security 
module.  NSS's zlib is not, and therefore any functional zlib is probably 
acceptable.

Comment 18

[Mikhail T.]

Well, SQLite is fine by me as long as you do not _ship your own_ :-) 
 

Comment 19

[Mikhail T.]

(In reply to comment #9)  
> That means, a DB made on a big endian 64-bit Unix box must work   
> if moved to a little endian 32-bit windows box (for example).  
  
I just checked. The db_dump185, that comes with SleepyCat's db4, compiled on  
FreeBSD/amd64 had no problems with the two .db files, that come with Firefox  
distribution:  
  
db_dump185-4.2  
mozilla/security/nss/tests/pkcs11/netscape/suites/security/ssl/cert7.db 
  
db_dump185-4.2  
mozilla/security/nss/tests/pkcs11/netscape/suites/security/ssl/key3.db 
 
There was no problem db_dump-ing any of the three .db files found in my ancient 
~/.netscape either. I think, you can use the db.h comfortably, if it is 
present. 
  

Comment 20

[Wan-Teh Chang]

Attachment: Proposed patch to use system zlib with Linux as an example

I propose adding two new configuration variables:
USE_SYSTEM_ZLIB and ZLIB_LIBS.	If a platform has
a suitable system zlib, we set USE_SYSTEM_ZLIB to
1 and ZLIB_LIBS to the linker command-line arguments
for the system zlib (e.g., -lz) in the platform's
config file in coreconf.  I did that in Linux.mk.
We can do the same for other platforms that have
system zlib.

Comment 21

[Wan-Teh Chang]

Comment on attachment 191608 [details] [diff] [review]
Proposed patch to use system zlib with Linux as an example

I forgot to mention that cmd/platlibs.mk defines JAR_LIBS
the same way in all the four cases, so in this patch I
factored out the common definition of JAR_LIBS and put it
at the end of the file.

I can change ZLIB_DIR to ZLIB_SRCDIR if you think the latter
is better.  (ZLIB_DIR means the location of the zlib
installation in some other projects.)

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 191608 [details] [diff] [review]
Proposed patch to use system zlib with Linux as an example

r=nelson@bolyard.  Good simplification.

Comment 23

[Wan-Teh Chang]

Attachment: Proposed patch to use system zlib with Linux as an example (as checked in)

I checked in the patch, after changing ZLIB_DIR to ZLIB_SRCDIR.

Now we need to figure out on what platforms we can use system
zlib.  We should at least look into these: Solaris, HP-UX,
Mac OS X, and *BSD.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

The Nightly build QA tests have been failing for Linux since this checkin.
The failure exactly matches the descriptions in 
https://bugzilla.mozilla.org/show_bug.cgi?id=131826#c15 and
https://bugzilla.mozilla.org/show_bug.cgi?id=131826#c16

So, Wan-Teh, please back out this one change:

Index: mozilla/security/coreconf/Linux.mk
===================================================================
RCS file: /cvsroot/mozilla/security/coreconf/Linux.mk,v
retrieving revision 1.20
diff -u -r1.20 Linux.mk
--- mozilla/security/coreconf/Linux.mk	9 Aug 2005 22:31:24 -0000	1.20
+++ mozilla/security/coreconf/Linux.mk	16 Aug 2005 01:07:58 -0000
@@ -172,3 +172,6 @@
 # Always set CPU_TAG on Linux, OpenVMS, WINCE.
 #
 CPU_TAG = _$(CPU_ARCH)
+
+USE_SYSTEM_ZLIB = 1
+ZLIB_LIBS = -lz

Comment 25

[Wan-Teh Chang]

Nelson: I backed out that change.  I spent today looking into
this, and have some preliminary findings.  The problem is that
when we unzip the jar file, the zlib 1.1.4 inflate(Z_FINISH)
function sometimes returns Z_BUF_ERROR (which means either
it needs more input, or there is no more space in
the output buffer) instead of Z_STREAM_END.  At that point
zs.avail_in is 0 and zs.avail_out is nonzero (if I make the output
buffer larger than necessary).

Comment 26

[Wan-Teh Chang]

The reason inflate(Z_FINISH) returns Z_BUF_ERROR is
that inflate_blocks returns Z_OK instead of the expected
Z_STREAM_END.  Again, this error is intermittent.

Comment 27

[Wan-Teh Chang]

Attachment: Patch for using zlib 1.1.4

I found the problem.  It turns out that raw inflate (which
is what signtool uses) in zlib 1.1.4 needs an extra dummy
byte at the end.  The need for the extra dummy byte was
eliminated in zlib 1.2.0.  See
- zlib change log, under "Changes in 1.2.0 (9 March 2003)":
  http://www.zlib.net/ChangeLog.txt
- java.util.zip.Inflater constructor, "nowrap" option:
 
http://java.sun.com/j2se/1.4.2/docs/api/java/util/zip/Inflater.html#Inflater(boolean)

- Comment in zlib's gzio.c source file:
  http://lxr.mozilla.org/security/source/security/nss/cmd/zlib/gzio.c#168

It seems that the author of lib/jar/jarfile.c knew about this requirement,
but he only went half way -- he allocates a buffer with room for the extra
dummy byte, but gives the size of just the compressed data, without the
extra dummy byte, to inflate().  It would be nice to look in the Netscape
internal NSS CVS repository to find out why he adds 1 to phy->length in
one place but not in the other.

My patch does not initialize the dummy byte.  I'll ask the zlib maintainers
if it's necessary to do so.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 193254 [details] [diff] [review]
Patch for using zlib 1.1.4

Does zlib 1.2.0 ignore this extra byte?  Does this extra byte cause the output
to contain an extra byte, or have any other undesirable side effect when used
with 1.2.x ?

Comment 29

[Wan-Teh Chang]

Nelson: all versions of zlib ignore the extra dummy byte.
The inflate() function returns Z_STREAM_END with zs.avail_in=1,
which means the extra dummy byte is still in the input stream.

I will submit a new patch that makes sure that the extra dummy
byte is not used (that is, zs.avail_in == 1 when inflate()
terminates with Z_STREAM_END).

I found another use of inflate() in lib/jar that should also
be fixed the same way.  We are (or rather appear to be) okay
there because we aren't testing for Z_STREAM_END there; we
terminate when we have fed all the bytes into inflate().

This bug opens a can of worms that I don't really have time to
handle :-(

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 193254 [details] [diff] [review]
Patch for using zlib 1.1.4

This patch looks OK to me as far as it goes. But given your comment that you've
already found another place that needs a similar fix, I think this patch is
effectively incomplete.
I look forward to the more complete patch.

Comment 31

[Wan-Teh Chang]

Glenn, I added you to the CC list because people told me you're
familiar with the zlib API.

NSS has a component that calls the zlib API.  I believe it is calling
some zlib functions incorrectly, so that it doesn't work with certain
older versions of zlib.  I will need your help to fix all such problems
in that NSS component.

Could you start by reviewing my (incomplete) patch in comment 27?
Thanks.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 27,
> It would be nice to look in the Netscape internal NSS CVS repository to 
> find out why he adds 1 to phy->length in one place but not in the other.

I looked in Sun's copy of that repository. :) The code in question is from 
the very first revision of that file, rev 1.1, Tue Dec 9 13:27:33 1997 UTC.
The only change to that code since then was on Sun May 31 08:39:47 1998 UTC
when Jamie added the argument named expected_out_len to jar_inflate_memory.
Prior to that, the expected output length was computed as (10 * *length);
So, I think the results of this search do not provide the explanation that
Wan-Teh desired.

Comment 33

[Wan-Teh Chang]

I'd like to try to fix this bug again.

I have written a new patch for interoperability with the old
zlib 1.1.4.  zlib 1.2.0 has been released for more than five
years now, so I'm not sure if we still need this patch.  But
it's harmless for zlib 1.2.x.

Last time I enabled the use of system zlib on Linux, it broke
signtool on Red Hat Advanced Server 2.1 (see comment 24).  If
all the versions of Linux we need to support are using zlib
1.2.x, we don't need this zlib 1.1.4 dummy byte patch.

Comment 34

[Wan-Teh Chang]

Attachment: Patch for using zlib 1.1.4, v2

I also edited Julien's fix for bug 373907 (limit manifest
size in JAR file).  For easier code review, I diff'ed
against the revision (1.6) before Julien's fix.

Comment 35

[Wan-Teh Chang]

Attachment: Use system zlib on Linux

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

WTC, Is it true that these patches affect only signtool?  
IINM, the patched code is not in any NSS shared lib, but only in signtool.
Do you concur?

Comment 37

[Wan-Teh Chang]

modutil is also linked with the patched code, but I
don't know if modutil actually uses it.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 335462 [details] [diff] [review]
Use system zlib on Linux

Wan-Teh, 
The name for patch attachment 192786 [details] [diff] [review] above says "(as checked in)".
But it appears to me that only part of that patch was committed,
and the purpose of this patch is to commit the one remaining part
of that original patch, which was to mozilla/security/coreconf/Linux.mk

Is that understanding correct?

Comment 39

[Wan-Teh Chang]

Yes, that's correct.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 335461 [details] [diff] [review]
Patch for using zlib 1.1.4, v2

I am giving this r+ .  In doing so, I am relying on comment 29, which says
> all versions of zlib ignore the extra dummy byte.
That is a necessary condition to this r+

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 335462 [details] [diff] [review]
Use system zlib on Linux

r=nelson

Comment 42

[Wan-Teh Chang]

I checked in these two patches on the NSS trunk (NSS 3.12.2).

Checking in jarfile.c;
/cvsroot/mozilla/security/nss/lib/jar/jarfile.c,v  <--  jarfile.c
new revision: 1.8; previous revision: 1.7
done

Checking in Linux.mk;
/cvsroot/mozilla/security/coreconf/Linux.mk,v  <--  Linux.mk
new revision: 1.35; previous revision: 1.34
done

Comment 43

[Wan-Teh Chang]

Attachment: Back out zlib 1.1.4 support (for future reference)

When we don't need to support zlib 1.1.4, apply this
patch to back out the code that adds the extra dummy
byte.