450845 - Stop exporting symbols that are not present in the .def files

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to stop using declspec in NSS libraries

Ever since NSS became shared libraries in release 3.2, we have been trying 
to control the symbols (functions mostly) that get "exported" from those 
shared libraries through the use of the .def files.  

Despite that, MANY function and data symbols that are NOT in the .def files
are exported from the NSS shared libraries on Windows, and perhaps also on 
BEOS, OS/2, & Symbian, and perhaps also on all systems with gcc version 3.3
and higher.

This is because NSS widely uses of a pair of NSPR macros, PR_IMPLEMENT 
and NSS_IMPLEMENT, which declare a function with _declspec(dllexport) 
attributes on Windows, BEOS, OS/2 and Symbian, and with 
__attribute__((visibility("default"))) attributes when built with gcc 
version 3.3 and later.   This is not to be seen as a fault of NSPR,
but as a misuse of the NSPR macros in NSS.

The use of these macros in function declarations has the effect of forcing 
the function to be exported from any shared library into which it is linked, 
regardless of the .def file.  Consequently, on Windows and the other systems
mentioned above, there are MANY symbols that are exported that we do not 
wish to have exported.  This means that our libraries do not export the same 
symbols on all platforms.

So, I propose to fix this by eliminating all use of those NSPR macros 
in NSS shared library source code and header files. 
I propose to do that with two changes.

1) Change the NSS_EXTERN and NSS_IMPLEMENT macros so that they no longer 
invoke the PR_EXTERN and PR_IMPLEMENT macros, and then

2) Replace all remaining invocations of PR_EXTERN and PR_IMPLEMENT with 
invocations of NSS_EXTERN and NSS_IMPLEMENT.

Attached is a patch that does this for NSS shared libraries.  
I expect my fellow module owners will want to tweak it a bit. 

It's conceivable to me that this fix might cause outcries that we have 
broken the NSS ABI.  If that happens, we can add symbols to the .def files 
to export symbols that are being used.  Personally, I have taken the stance
for a long time now that NSS's public API is defined by the .def files, 
and only symbols whose names do not begin with underscores in those files.  
Anyone who relies on symbols not defined there is relying on private APIs
which are not covered by the ABI backward compatiblity promises we make.

With use of NSS apparently increasing, I'd rather stop the unintentional 
export of symbols sooner, rather than later.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: complete patch

sorry, that last patch was incomplete.

This patch includes changes to files under nss/lib/ckfw, files that 
may not normally be part of an NSS build.  They are
lib/ckfw/capi/staticobj.c
lib/ckfw/nssmkey/staticobj.c

I don't know if the libraries built from those directories have .def files
nor whether the symbols that I have changed are intended to be exported 
from those libraries, or not.  

Bob, please advise concerning those directories.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Two additional comments.  
1) There are some uses of _declspec(dllexport) in source files in 
security/nss/cmd/zlib/zconf.h

2) there are some uses of _declspec(dllimport) in various NSS source files.
I have not removed any of those.  I don't believe they are a problem.  
See a listing of them in this bug's URL citation.

Comment 3

[Wan-Teh Chang]

Comment on attachment 334066 [details] [diff] [review]
complete patch

We don't need the NSS_EXTERN, NSS_EXTERN_DATA, NSS_IMPLEMENT,
and NSS_IMPLEMENT_DATA macros.  They should be deleted or
marked deprecated.

At least, don't replace PR_IMPLEMENT by NSS_IMPLEMENT, etc.
Just delete PR_IMPLEMENT.

Comment 4

[Robert Relyea]

> This patch includes changes to files under nss/lib/ckfw, files that 
> may not normally be part of an NSS build.  They are
> lib/ckfw/capi/staticobj.c
> lib/ckfw/nssmkey/staticobj.c

There should be no publically exported symbols from those files. both capi and nssmkey have .def files exporting the PKCS #11 C_GetFunctionList function.

bob

Comment 5

[Robert Relyea]

Comment on attachment 334066 [details] [diff] [review]
complete patch

r+ rrelyea.

I'm also OK with just deleting the definitions as wtc suggested.

bob

Comment 6

[Wan-Teh Chang]

Comment on attachment 334066 [details] [diff] [review]
complete patch

Another reason for not replacing PR_IMPLEMENT by NSS_IMPLEMENT
(in lib/util/{nssrwlk.c,utf8.c}) is that this makes lib/util
depend on lib/base.  I think this dependency is bad, and is
a kind of layering violation (lib/base is to "Stan" what lib/util
is to NSS).  (In fact, this patch doesn't compile because you
didn't add #include "nssbaset.h" to lib/util/{nssrwlk.c,utf8.c}.)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

The patch did compile for me.  I determined which header files needed to 
have the #include added by clobbering and recompiling until I got a clean
build.  At least, I think that's what I did.  Maybe I should try another
clobber build to see.  

Comment 8

[Wan-Teh Chang]

Comment on attachment 334066 [details] [diff] [review]
complete patch

You're right.  The patch should compile.  I didn't apply
the changes to lib/util/{nssrwlk.h,secport.h}.  In any
case, lib/util ("NSS Classic") and lib/base ("Stan")
should stay independent of each other.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Eliminate PR_EXTERN, PR_IMPLEMENT in nss/lib (checked in)

Unlike the previous patch, which attempted to replace PR_EXTERN with 
NSS_EXTERN and PR_IMPLEMENT with NSS_IMPLEMENT, this patch just eliminates
the references to PR_EXTERN and PR_IMPLEMENT in nss/lib only.

Like the previous patch, this patch makes no attempt to eliminate the
thousands of existing references to NSS_EXTERN, NSS_EXTERN_DATA, 
NSS_IMPLEMENT and NSS_IMPLEMENT_DATA.  Those symbols simply no longer 
expand to references to their NSPR equivalents.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 341060 [details] [diff] [review]
Eliminate PR_EXTERN, PR_IMPLEMENT in nss/lib (checked in)

This patch also does not add dependencies to header files in nss/lib/base into other files. 

Wan-Teh, I believe this is the patch you requested previously in this bug.  
Please review this patch.

Comment 11

[Wan-Teh Chang]

Comment on attachment 341060 [details] [diff] [review]
Eliminate PR_EXTERN, PR_IMPLEMENT in nss/lib (checked in)

r=wtc.

In lib/softoken/pkcs11t.h, you should use "type" or "rtype"
instead of "rtyp", or just leave "rv" unchanged.  ("type"
is not a reserved word in C or C++, and this is only used
by the C preprocessor anyway.)

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 341060 [details] [diff] [review]
Eliminate PR_EXTERN, PR_IMPLEMENT in nss/lib (checked in)

base/nssbaset.h;          new revision: 1.7;  previous revision: 1.6
ckfw/capi/staticobj.c;    new revision: 1.2;  previous revision: 1.1
ckfw/nssmkey/staticobj.c; new revision: 1.2;  previous revision: 1.1
softoken/pkcs11t.h;       new revision: 1.19; previous revision: 1.18
util/nssb64d.c;           new revision: 1.7;  previous revision: 1.6
util/nssrwlk.c;           new revision: 1.9;  previous revision: 1.8
util/nssrwlk.h;           new revision: 1.4;  previous revision: 1.3
util/secport.h;           new revision: 1.17; previous revision: 1.16
util/utf8.c;              new revision: 1.13; previous revision: 1.12

Comment 13

[Julien Pierre]

Nelson,

Should this bug be marked resolved ?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

> Should this bug be marked resolved ?
alas, no.  :(

We removed the vast majority of undesired exported symbols, I think,
but sadly, one function that seems to be exported from nss3.dll, 
even though it's not in nss.def, is mktemp.  :(
This has profoundly nasty implications, I fear.

so far, I have only checked nss3.dll.  
I think I will write a script to check all of NSS's DLLs.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

I wrote a little hack shell script to find the symbols being exported from
DLLs that are not defined in the respective .def files.
This shell script is run in the mozilla/dist/WIN*OBJ*/lib directory.

cp `find ../../../security -name \*.def -print | grep OBJ` .
cp nssckbi.dll nssckbi3.dll
for a in freebl nssckbi nssutil softokn ssl nss nssdbm smime sqlite
do echo === Library $a === ;
dumpbin /exports ${a}3*.dll | tail- -l +20 | sed -e 's/  */ /g' | cut -f 5 -d ' ' -s | grep -v '^$' > ${a}.exports; 
sed -e 's/;.*//' -e 's/ DATA.*//' ${a}.def | grep -v '^$' | grep -v ^EXPORTS | grep -v ^LIBRARY >> ${a}.exports;
sort < ${a}.exports | uniq -u ;
done

This script outputs a series of lines saying === Library XXXXXXX ===
followed by any symbols being exported from that library but not in the 
corresponding .def file.  The results are as follows:

=== Library freebl ===
=== Library nssckbi ===
=== Library nssutil ===
=== Library softokn ===
=== Library ssl ===
=== Library nss ===
mktemp
=== Library nssdbm ===
dbopen
mktemp
=== Library smime ===
=== Library sqlite ===

So, we're not out of the woods yet.  We must figure out how to stop 
exporting mktemp and dbopen from those libraries.

I'll bet that we're including some windows header files that declare
those functions with dllexport  :(

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Correction:

cp `find ../../../security -name \*.def -print | grep OBJ` .
cp nssckbi.dll nssckbi3.dll
for a in freebl nssckbi nssutil softokn ssl nss nssdbm smime sqlite
do echo === Library $a === ;
dumpbin /exports ${a}3*.dll | tail -l +20 | sed -e 's/  */ /g' | \
  cut -f 5 -d ' ' -s | grep -v '^$' > ${a}.exports; 
sed -e 's/;.*//' -e 's/ DATA.*//' ${a}.def | grep -v '^$' | \
  grep -v ^EXPORTS | grep -v ^LIBRARY >> ${a}.exports;
sort < ${a}.exports | uniq -u ;
done

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

OK, so we missed a PR_EXTERN macro invocation in 
mozilla/dbm/include/mcom_db.h 
That explains why dbopen is exported from nssdbm3.dll.  

Perhaps mktemp is explained by this: In <io.h> mktemp is declared as:

_CRT_NONSTDC_DEPRECATE(_mktemp) _CRT_INSECURE_DEPRECATE(_mktemp_s) 
_CRTIMP char * __cdecl mktemp(__inout_z char * _TemplateName);

and _CRTIMP expands to  __declspec(dllimport)

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

mktemp is being intentionally exported from libnss for backwards 
compatiblity.  The same problem now afflicting nssdbm previously 
afflicted libnss.  unfortunately, mktemp wasn't added to nss.def.

The question remains: how do we get mktemp to cease to be an exported
symbol of nssdbm.  I think maybe the answer is to use real mktemp (?)
and stop having a private implementation in dbm.  Need to check that.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 21

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.