Last Comment Bug 142575 - 3.4.2 not backwards compatible - missing mktemp
: 3.4.2 not backwards compatible - missing mktemp
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.4
: x86 Windows 2000
: P1 normal (vote)
: 3.4.2
Assigned To: Wan-Teh Chang
: Bishakha Banerjee
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-05-06 10:50 PDT by rweltman
Modified: 2008-11-21 01:45 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch to add function forwarders to nss3.dll that call the dbopen and mktemp functions exported from softokn3.dll (1.11 KB, patch)
2002-05-22 17:40 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Solution 1: forwarding mktemp to Windows' implentation: use the /export linker pragma (1.15 KB, patch)
2002-05-23 14:45 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Solution 2: forwarding mktemp to Windows' implentation: simulate how mktemp was exported (1.24 KB, patch)
2002-05-23 14:53 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Patch to add a function forwarder to nss3.dll that calls the mktemp function exported from softokn3.dll (1.02 KB, patch)
2002-05-24 14:32 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Final solution: export mktemp (which calls _mktemp) from nss3.dll but do not put mktemp in nss3.lib (1.88 KB, patch)
2002-05-28 12:31 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description rweltman 2002-05-06 10:50:57 PDT
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.
Comment 1 Mark Smith (:mcs) 2002-05-06 11:41:33 PDT
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.
Comment 2 Wan-Teh Chang 2002-05-07 13:35:27 PDT
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.
Comment 3 Wan-Teh Chang 2002-05-07 14:21:24 PDT
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.
Comment 4 Robert Relyea 2002-05-07 16:44:29 PDT
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?
Comment 5 Wan-Teh Chang 2002-05-07 17:18:29 PDT
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.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2002-05-22 13:27:56 PDT
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?
Comment 7 Wan-Teh Chang 2002-05-22 13:43:46 PDT
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.
Comment 8 Wan-Teh Chang 2002-05-22 17:40:15 PDT
Created attachment 84704 [details] [diff] [review]
Patch to add function forwarders to nss3.dll that call the dbopen and mktemp functions exported from softokn3.dll

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.
Comment 9 Wan-Teh Chang 2002-05-22 17:50:25 PDT
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.
Comment 10 Wan-Teh Chang 2002-05-22 17:52:48 PDT
The problem of missing mktemp (and dbopen) first appeared in NSS 3.4.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2002-05-22 20:51:50 PDT
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.
Comment 12 Mark Smith (:mcs) 2002-05-23 06:25:59 PDT
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.
Comment 13 Wan-Teh Chang 2002-05-23 14:45:13 PDT
Created attachment 84824 [details] [diff] [review]
Solution 1: forwarding mktemp to Windows' implentation: use the /export linker pragma

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")
Comment 14 Wan-Teh Chang 2002-05-23 14:53:40 PDT
Created attachment 84827 [details] [diff] [review]
Solution 2: forwarding mktemp to Windows' implentation: simulate how mktemp was exported

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.
Comment 15 Wan-Teh Chang 2002-05-23 15:51:26 PDT
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.
Comment 16 Nelson Bolyard (seldom reads bugmail) 2002-05-24 14:30:52 PDT
It appears that this patch has caused the nightly build to fail on NT
when linking certcgi.exe
Comment 17 Wan-Teh Chang 2002-05-24 14:32:42 PDT
Created attachment 84958 [details] [diff] [review]
Patch to add a function forwarder to nss3.dll that calls the mktemp function exported from softokn3.dll

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.
Comment 18 Wan-Teh Chang 2002-05-28 12:31:33 PDT
Created attachment 85291 [details] [diff] [review]
Final solution: export mktemp (which calls _mktemp) from nss3.dll but do not put mktemp in nss3.lib

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.
Comment 19 Wan-Teh Chang 2002-05-28 12:37:44 PDT
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.
Comment 20 Nelson Bolyard (seldom reads bugmail) 2008-11-21 01:45:40 PST
This work should have added the mktemp symbol to nss.def, but did not.

Note You need to log in before you can comment on or make changes to this bug.