Closed Bug 142575 Opened 22 years ago Closed 22 years ago

3.4.2 not backwards compatible - missing mktemp

Categories

(NSS :: Libraries, defect, P1)

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rweltman, Assigned: wtc)

Details

Attachments

(2 files, 3 obsolete files)

It was probably not a good idea to have exported mktemp in earlier NSS versions, 
e.g.

C:\WINNT\system32>dumpbin /exports nss3.dll | grep mktemp
        445  1BC 00084946 mktemp

But removing it in 3.4.2 Beta causes applications to no longer start. e.g. 
ldapsearch.exe:

"The procedure entry point mktemp could ot be located in the dynamic link 
library nss3.dll"

If nss3.dll must be made backwards incompatible, it should have a new name so it 
can coexist with versions that support older applications.
A side comment: the specific problem with the LDAP command line utilities
(ldapsearch et al) was fixed as part of this bug fix:

http://bugzilla.mozilla.org/show_bug.cgi?id=138627
mktemp() warnings when building ldapsearch.exe

But it would certainly be better if none of the NSS DLLs exported functions that
are not part of the public, supported API.
Thanks for the bug report.  This is an interesting bug.

On Windows, the nss3.dll in NSS 3.3.x or older exports
two DBM symbols (dbopen and mktemp) by accident.  In
NSS 3.4 the DBM code was moved to the new softokn3.dll.
This is why nss3.dll in NSS 3.4 does not export mktemp.

Looking at the DBM source code, I can see why dbopen
is DLL exported (dbopen is defined with PR_IMPLEMENT),
but I haven't figured out why mktemp is DLL exported.
It is defined like this, without the PR_IMPLEMENT macro:

char *
mktemp(char *path)
{
        return(_gettemp(path, (int *)NULL, 0) ? path : (char *)NULL);
}

It is possible that one of the header files we include
declares mktemp with the _declspec(dllexport) keyword.

As to whether nss3.dll should be bug-compatible with
the DBM symbols (dbopen and mktemp) that it accidentally
exported on Windows before, I think it is not necessary.
In particular dbopen will require including the whole
DBM library redundantly in nss3.dll.
OK, I found out why mktemp is DLL exported on Windows.

mozilla/dbm/src/mktemp.c includes "mcom_db.h", which in
turn includes <io.h>.  The standard header file <io.h>
has:

_CRTIMP char * __cdecl mktemp(char *);

where _CRTIMP is defined as:

#define _CRTIMP __declspec(dllimport)

Because of this declaration of mktemp in <io.h>, when we
compile mozilla/dbm/src/mktemp.c, we have:

gmake[1]: Entering directory `D:/nss-tip/mozilla/security/dbm/src'
cl -FoWINNT5.0_DBG.OBJ/mktemp.obj -c -Od -Z7 -MD -W3 -nologo -GT -DXP_PC -DDEBUG
 -D_DEBUG -UNDEBUG -DDEBUG_wtc -DWIN32 -D_WINDOWS -D_X86_ -DWINNT -DSTDC_HEADERS
 -DHAVE_STRERROR -DHAVE_SNPRINTF -DMEMMOVE -D__DBINTERFACE_PRIVATE  -I../../../d
ist/WINNT5.0_DBG.OBJ/include  -I../../../dist/public/dbm -I../../../dist/private
/dbm -I../../../dbm/include  ../../../dbm/src/mktemp.c
mktemp.c
../../../dbm/src/mktemp.c(93) : warning C4273: 'mktemp' : inconsistent dll linka
ge.  dllexport assumed.
This is a bit disturbing. is there any options we can give the linker to
guarrentee that *ONLY* those symbols included in *.def are exported?
The command-line options for LINK are listed in the Linker Reference
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/HTML/_core_linker_reference.asp

I don't see any options that guarrentee that *ONLY* those symbols
included in *.def are exported.

I don't see anything in the Module-Definition (.DEF) Files page
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/HTML/_core_module.2d.definition_files.asp
that does that either.
Priority: -- → P1
Target Milestone: --- → 3.6
In comment 2 above, Wan-Teh wrote:

> As to whether nss3.dll should be bug-compatible with
> the DBM symbols (dbopen and mktemp) that it accidentally
> exported on Windows before, I think it is not necessary.

One of the motivations for making NSS be a DLL in the first place was so 
that we could fix bugs in NSS, and servers in the field could get those
fixes by just getting a new NSS DLL, without also updating their server
executable.  Also, multiple applications were supposed to be able to 
share a single set of NSPR and NSS DLLs.  

Now, imagine that some server release contains 3.4 or 3.4.1, and a user
installs it on a box that also has other Netscape or iPlanet servers 
running.  The newer 3.4.x NSS DSOs replace the older NSS DLLs (3.2, say).
Now, some of the other server products that share these DLLs won't start.

This is particularly a problem on Windows platforms.  IINM, the choice
of the DLL that is used to resolve a symbol is fixed at link time, being
determined from the "import library".  So, later, if that symbol is missing
from one DLL, but is present in another, it will still appear to be a 
missing symbol, preventing the application from running.

> In particular dbopen will require including the whole
> DBM library redundantly in nss3.dll.

Couldn't nss3.dll just call the dbopen function exported from softoken.dll?
Nelson wrote:
> Couldn't nss3.dll just call the dbopen function exported
> from softoken.dll?

I remember that's possible on Windows.  I'll ask around if anyone
knows how to do that.
This page describes how to add function forwaders to a DLL:
http://www.microsoft.com/msj/defaulttop.asp?page=/msj/archive/s202b.htm.

I verified that this patch fixes the "Entry Point Not Found" error
for ldapsearch.exe.  (The error message is "The procedure entry point
mktemp could not be located in the dynamic link library nss3.dll.")

I am still not sure if we should check in this patch to be backward
compatible with this bug.
Changed target milestone to 3.4.2.

We should decide on one of the options before NSS 3.4.2 is released.

Option 1. Be backward compatible with the bug.  Add the function
forwarders to nss3.dll in NSS 3.4.2.

Option 2. Fix the bug.  Do not export dbopen and mktemp from
softokn3.dll in NSS 3.6.
Status: NEW → ASSIGNED
Target Milestone: 3.6 → 3.4.2
The problem of missing mktemp (and dbopen) first appeared in NSS 3.4.
Version: 3.4.2 → 3.4
Those forwarders are a very interesting find!  

In this case, I vote for bug compatibility, since the alternative is a
server that won't even start!  

We might consider forwarding mktemp to Windows' implentation instead of to 
our own.
I have mixed feelings about this. At some point you need to fix the problem.
Ideally that would be done at the same time the NSS DLLs/so's are renamed.
Nelson's suggestion of forwarding mktemp to Windows' implentation
is a good one.	Ideally I want to say:
#pragma comment(linker, "/export:mktemp=msvcrt._mktemp")
But we also use the debug C run-time library msvcrtd.dll and I
don't have a good way to determine whether to forward to
msvcrt.dll or msvcrtd.dll at compile time.  So I need other
solutions.

This is the first solution I came up with.  It is more elegant
and seems to work.
#pragma comment(linker, "/export:mktemp=__imp___mktemp")
The second solution simulates how mktemp was accidentally exported
in NSS 3.2.x and 3.3.x.  This solution is less elegant but I am
more confident that it is backward compatible with the bug.  So I
propose we check in this solution.

Note that I decided not to export dbopen.  (Code that accidentally
uses the dbopen implementation in NSS is problematic anyway.)  I
don't think we should volunteer to add the dbopen forwarder unless
there is a need for it.
Solution 2 (attachment 84827 [details] [diff] [review]) has been checked into the
tip of NSS, NSS_3_4_BRANCH, and NSS_3_5_BRANCH.

I filed bug 146552 to remind us to remove this
"bug compatibility" fix in NSS 4.0.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
It appears that this patch has caused the nightly build to fail on NT
when linking certcgi.exe
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It turns out that Solution 2 (attachment 84827 [details] [diff] [review]) works but breaks
the NSS build (when linking certcgi.exe) because _mktemp is multply
defined in nss.lib(nssinit.obj) and dbm.lib(mktemp.obj).  It is
possible to fix this problem: put the mktemp function in a separate
.c file that is included in nss3.dll but is not included in nss.lib.
This requires some makefile work.

Solution 1 (attachment 84824 [details] [diff] [review]) doesn't work.  It crashes at run time.

The original solution (forwarding to mktemp in softokn3.dll) works
and is simple (no makefile change is required), so I decided to use
it in NSS 3.4.2.  This patch is what actually got checked in.
Attachment #84704 - Attachment is obsolete: true
Attachment #84824 - Attachment is obsolete: true
Attachment #84827 - Attachment is obsolete: true
After testing several possible solutions, I finally decided
on this patch.	A mktemp function that calls _mktemp in the
C run-time library is exported from nss3.dll to be backward
compatible with old binaries, but mktemp is not placed in the
import library nss3.lib to prevent new binaries from resolving
their references to mktemp with the mktemp function in nss3.dll.
The "final solution" patch has been checked into the tip of NSS,
NSS_3_4_BRANCH, and NSS_3_5_BRANCH.

I tested it with a test program built in exactly the same way the
ldapsearch.exe program in the LDAP C SDK is built.  I verified
that the old binary (linked with the old nss3.dll) works with the
new nss3.dll, which calls _mktemp in the C run-time library, and
that the new binary (linked with the new nss3.dll) resolves its
reference to mktemp with the C run-time library, not nss3.dll.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
This work should have added the mktemp symbol to nss.def, but did not.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: