Closed
Bug 142575
Opened 22 years ago
Closed 22 years ago
3.4.2 not backwards compatible - missing mktemp
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.4.2
People
(Reporter: rweltman, Assigned: wtc)
Details
Attachments
(2 files, 3 obsolete files)
1.02 KB,
patch
|
Details | Diff | Splinter Review | |
1.88 KB,
patch
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
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.
Assignee | ||
Comment 2•22 years ago
|
||
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.
Assignee | ||
Comment 3•22 years ago
|
||
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•22 years ago
|
||
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?
Assignee | ||
Comment 5•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Priority: -- → P1
Target Milestone: --- → 3.6
Comment 6•22 years ago
|
||
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?
Assignee | ||
Comment 7•22 years ago
|
||
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.
Assignee | ||
Comment 8•22 years ago
|
||
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.
Assignee | ||
Comment 9•22 years ago
|
||
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
Assignee | ||
Comment 10•22 years ago
|
||
The problem of missing mktemp (and dbopen) first appeared in NSS 3.4.
Version: 3.4.2 → 3.4
Comment 11•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 13•22 years ago
|
||
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")
Assignee | ||
Comment 14•22 years ago
|
||
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.
Assignee | ||
Comment 15•22 years ago
|
||
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
Comment 16•22 years ago
|
||
It appears that this patch has caused the nightly build to fail on NT when linking certcgi.exe
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 17•22 years ago
|
||
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
Assignee | ||
Comment 18•22 years ago
|
||
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.
Assignee | ||
Comment 19•22 years ago
|
||
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 ago → 22 years ago
Resolution: --- → FIXED
Comment 20•16 years ago
|
||
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.
Description
•