3.22 KB, patch
|Details | Diff | Splinter Review|
2.93 KB, patch
Nelson Bolyard (seldom reads bugmail): review+
|Details | Diff | Splinter Review|
487 bytes, patch
Nelson Bolyard (seldom reads bugmail): review+
|Details | Diff | Splinter Review|
2.62 KB, patch
|Details | Diff | Splinter Review|
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
Created attachment 190974 [details] [diff] [review] 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.
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.
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 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 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.
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?
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.
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.
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.
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.
(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).
(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?
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
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 .
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.
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.
Well, SQLite is fine by me as long as you do not _ship your own_ :-)
(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.
Created attachment 191608 [details] [diff] [review] 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 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 on attachment 191608 [details] [diff] [review] Proposed patch to use system zlib with Linux as an example r=nelson@bolyard. Good simplification.
Created attachment 192786 [details] [diff] [review] 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.
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
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).
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.
Created attachment 193254 [details] [diff] [review] 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 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 ?
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 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.
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.
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.
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.
Created attachment 335461 [details] [diff] [review] 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.
Created attachment 335462 [details] [diff] [review] Use system zlib on Linux
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?
modutil is also linked with the patched code, but I don't know if modutil actually uses it.
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?
Yes, that's correct.
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 on attachment 335462 [details] [diff] [review] Use system zlib on Linux r=nelson
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
Created attachment 337212 [details] [diff] [review] 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.