Closed Bug 286642 Opened 20 years ago Closed 17 years ago

util should be in a shared library

Categories

(NSS :: Libraries, defect, P2)

Product:
NSS
Component:
Libraries
Version:
3.9.5
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

Attachments

(5 files, 17 obsolete files)

Fix upgrade db crash problem, Solaris AMD64 build
150.45 KB, patch
nelson
: review+
Details | Diff | Splinter Review
diff of attachment 283309 (implement feedback from Nelson) and attachment 284230 (fix upgrade DB crash problem)
49.93 KB, text/plain
Details
Test 1
152.98 KB, patch
Details | Diff | Splinter Review
Test 2
154.55 KB, patch
Details | Diff | Splinter Review
Don't export SGNDigestInfoTemplate from libnssutil3. Copy it to cryptohi and softoken where used. (checked in)
6.21 KB, patch
nelson
: review+
Details | Diff | Splinter Review
The util directory gets linked multiple times, to libnss3, libsoftokn3, fortezza, the root cert module . We should have only one copy of this code, perhaps in its own shared library, or we should add it to one of our existing shared libraries.
Blocks: 286643
Assignee: wtchang → saul.edwards.bugs
The public functions in lib/util are now exported via the nss3 shared library/DLL. On some platforms, for example, Windows, linking with a symbol means linking with a symbol in a particular shared library/DLL. So the nss3 shared library/DLL needs to continue to export the public functions in lib/util. On Windows, this can be achieved by function forwarders. There may be other solutions. It is extra work we need to do. Given this, I am not sure if we still want to make lib/util a shared library.
Wan-Teh, I think we should to the work required to fix this. The problem isn't just code duplication. There is state in lib/util, for example the dynamic oid table, which should be shared by all its users.
I'm not so sure this IS the right thing to do. Applications *ALWAYS* access util through libnss3. The PKCS #11 modules, on the otherhand, try to avoid dependencies on other libraries, particularly other libraries that may or may not be installed with them. That is way they don't include NSPR. The actual code duplications is actually pretty small, and in fact things like to OID tables *SHOULDN'T* affect the running of the PKCS #11 modules. Also, fortezza should be removed from the list because we've removed it from the build. bob
Oh, one other thing. If we do make libutil a separate shared library, then we need to strip it down to just those things that are really shared between libnss and the pkcs #11 module.
Bob, Re: comment #3 , libsoftokn3.so already links dynamically against libnspr4.so, libplc4.so and libplds4.so . I don't see how adding a dependency to libnssutil.so would be any different. Re: the OID table behavior, I disagree that the state of libnssutil shouldn't affect the softokn runtime. I think it's actually a desirable feature. But I will agree that a particular state for libutil shouldn't be required prior to C_Initialize; ie. the PKCS#11 modules that use libnssutil should continue to be able to operate without libnss3 .
1) Fortezza and builtins *DO NOT* have any other library dependencies. 2) The PKCS #11 module should not have any back channel hidden changes based on the environment of the client. It SHOULD NOT be affected by changes in the application view of libutil. The only reason the PKCS #11 code doesn't have it's own copy of all the stuff is for mantainability. If that is causing confusion, we can fork libutil betweent the PKCS #11 version and the version used by nss3.dll. We spend a lot of time making sure that the PKCS #11 modules are independent from the application. Yes we can do more to increase that independence... what we shouldn't do us reverse the trend. Is there something else driving this, or is this 'I think it should be this way', because if there isn't we should look at more profitable things to work on. bob
Bob, The fortezza module is going away, and I don't think we should spend more time discussing it. The builtins module IMO should depend on nspr shared libraries, rather than using the static library it currently has. I consider having multiple versions of NSPR in a process to be a bug. I think there is a bugzilla for that in fact; at least I remember Wan-Teh being involved in a discussion about this recently. I think the problem is where you draw the line between application and components. There is nothing in the PKCS#11 specification that dictates that you implement a PKCS#11 module as a single shared library. Many other vendors create libraries with dependencies on several DLLs. PKCS#11 libraries also use OS services all the time, even though they aren't bundled with it. Our helper functions in libutil shouldn't be off-limits. For the NSS PKCS#11 modules there are only two cases - when they are used in an NSS application, or in a non-NSS application. The first case will always work because all the DLLs are distributed together. The later case will still work, as long as we continue to distribute the DLLs our modules depend on. Eg. libsoftokn3 works today with otherwise pure Java applications that don't load libnss3 at all, because we distribute libnspr4.so, et al. And it will continue to work when we add the dependency to libnssutil.so . As far as what motivated this, I think it is mostly about being able to debug the code easier - ie. not having the debugger constantly ask "which one of the 3 copies of PORT_whatever do you want to set the breakpoint in today", and being able to make changes to the util functions once without having to rebuild several other DLLs, which happens relatively frequently as part of our performance improvement efforts . Forking the util source would be the wrong answer to this problem .
I think we are fundamentally at odds here. We don't even agree on the fundamentals. The decision not to make util a shared library was deliberate. I've given the reasons. Revisiting this is a bad idea IMHO. I think the better use of time would be to remove libutil dependencies from the modules. bob
QA Contact: bishakhabanerjee → jason.m.reid
In light of the recent decision to make ckbi depend on NSPR shared libs, I think it's time to revisit this issue. The whole point of this bug is to lessen code duplication between shared libs. Much of the code in lib/util is now duplicated in various shared libs. Changing from duplicated code to use of a shared lib need not constitute any sort of side channel between application and pkcs11 module. It merely lessens duplicated code and data. (I'm disagreeing with comment 2 here. OTOH, I see no value in forcing multiple dynamic OID tables. And sharing free lists of memory, such as the arena free list, is a GOOD thing.) Duplicating code doesn't necessarily make a PKCS11 module more independent of the application, but it certainly increases code working set size, increasing cache misses. No argument is given against sharing malloc and free (which share state), so why is sharing PORT_Alloc and PORT_Free bad? If there are specific functions in lib/util that one believes ought not to be shared, I'd like to read specifics about them. Perhaps a few of the routines in lib/util are stateful, and should not be shared. But I think they're in the extreme minority, and the existence of a few of them should NOT stop us from sharing the rest. The one previously given argument against making libutil shared which which I had no disagreement was the increased dependency on the use of NSPR's locks. And now it seems that we've decided to embrace that use in nssckbi, (unless it causes a problem for some existing NSS app, which seems unlikely). If we need function forwarders in libnss3 on windows, then IMO, so be it.
No longer blocks: 286643
This proposal is fine. I am just pointing out that on some platforms we may need to continue to export lib/util functions from the "nss3" shared library. This is true for Windows, where each symbol reference includes the DLL the symbol is defined in. On Windows this problem can be solved by symbol forwarding (e.g., nss3.dll would forward PORT_Malloc calls to the new nssutil3.dll, where PORT_Malloc is actually defined). Not all code in lib/util is shared. For example, secasn1d.c, secasn1e.c, and quickder.c are not used by the softoken and nssckbi modules, and they probably should not be moved to the new "nssutil3" shared library and should remain in the "nss3" shared library. I don't have a strong opinion though.
I see not problem with sharing certain things, but I still fundamentally disagree that that one version of softoken should be have differently than another based on sharing the dynamic OID Table! Sharing the heap or freelists or arena does not affect the semantics of softoken. other than resource constraints, the fact that they are shared does not affect how softoken runs. Sharing the dynamic OID Table does! It means that softoken will work with the same database in one application but may not work with another. On the other hand one can argue that softoken shouldn't be using the OID table at all! Other than that the only thing we need to be careful about is that util does not depend on anything in nss proper. (Hashing is what comes to mind first). bob
Do we want to fix this for 3.11 ?
I had a discussion with Nelson yesterday about this. It was suggested that we create functions that would access only the static part of the OID table, and not the dynamic part. The softoken would call those functions. We need to make a decision about whether to do make this change for 3.11 . We won't be able to add a util shared library in a 3.11.x patch release, so this enhancement would be latered to 3.12 if we make that call.
Setting target to 3.12 per our meeting today. Who should we reassign this to now that Saul has left ?
Priority: -- → P2
Target Milestone: --- → 3.12
QA Contact: jason.m.reid → libraries
Assignee: saul.edwards.bugs → nobody
I've seen the light, been converted, etc. The following wiki has some proposals for how we can do the conversion: http://wiki.mozilla.org/NSS_SHARED_UTIL After initial consultations with the rest of the team, we are currently on the trajectory of using Approach 1. The one thing that could ax the use of Approach 1 is running into binary compatibility issues. That would be the focus of this bug is to determine whether or not there will be binary compatibility issues. I'll attach a patch shortly that implements Approach 1. It's only been tested on Linux... so wider testing is needed as well. bob
Bob, Are you sure approach 1 is going to work for Windows ? I will test your patch when you attach it. Approach 1 probably won't work for OS/2, but I don't think we care about binary compatibility as much there since NSS on OS/2 is shipped with the browser only and not as a shared component, so the primary concern would be Windows.
Testing and comments on other platforms welcome. bob
My concern in comment 16 was addressed in comment 1 by Wan-Teh. We need function forwarders for Windows to preserve binary compatibility. This can be done on OS/2 as well. There is one more question here. If we are going to decouple libnss and libsoftoken (bug 385151), which version for libnssutil will be used ?
Attached patch Missing .def and .rc files (obsolete) — — Splinter Review
These go with the previous patch. They are necessary to build libutil.so
from the mozilla-nssdev mailing list. Relevant to this bug posted by wtc: On 6/8/07, Robert Relyea <rrelyea@redhat.com> wrote: > > Evaluations and recommendations for util can be found here: > http://wiki.mozilla.org/NSS_SHARED_UTIL I also like approach 1. On Windows you need to add the following to nss.def: PORT_Alloc=nssutil3.PORT_Alloc,PRIVATE PORT_Free=nssutil3.PORT_Free,PRIVATE ... These forward the functions to the same-named functions in nssutil3.dll, but do not put these functions in the import library nss3.lib. See http://msdn2.microsoft.com/en-us/library/hyx1zcd3(VS.80).aspx https://bugzilla.mozilla.org/show_bug.cgi?id=142575 Wan-Teh
In our .def files the line would be: PORT_Alloc=nssutil3.PORT_Alloc,Private ;- So it doesn't get picked up by Unix platforms
Bob, Yes, I saw that. But I don't see it in your patch. I tried it on Windows and the build is broken linking nssutil. link -nologo -DLL -SUBSYSTEM:WINDOWS -PDB:NONE -DEBUG -OUT:"WINNT5.2_DBG.OBJ/nss util3.dll" -DEF:WINNT5.2_DBG.OBJ/nssutil.def -EXPORT:mktemp=nss_mktemp,PRIVATE - MAP WINNT5.2_DBG.OBJ\\quickder.obj WINNT5.2_DBG.OBJ\\secdig.obj WINNT5.2_DBG.OB J\\derdec.obj WINNT5.2_DBG.OBJ\\derenc.obj WINNT5.2_DBG.OBJ\\dersubr.obj WINNT5. 2_DBG.OBJ\\dertime.obj WINNT5.2_DBG.OBJ\\nssb64d.obj WINNT5.2_DBG.OBJ\\nssb64e.o bj WINNT5.2_DBG.OBJ\\nssrwlk.obj WINNT5.2_DBG.OBJ\\nssilock.obj WINNT5.2_DBG.OBJ \\nsslocks.obj WINNT5.2_DBG.OBJ\\portreg.obj WINNT5.2_DBG.OBJ\\pqgutil.obj WINNT 5.2_DBG.OBJ\\secalgid.obj WINNT5.2_DBG.OBJ\\secasn1d.obj WINNT5.2_DBG.OBJ\\secas n1e.obj WINNT5.2_DBG.OBJ\\secasn1u.obj WINNT5.2_DBG.OBJ\\secitem.obj WINNT5.2_DB G.OBJ\\secoid.obj WINNT5.2_DBG.OBJ\\sectime.obj WINNT5.2_DBG.OBJ\\secport.obj WI NNT5.2_DBG.OBJ\\secinit.obj WINNT5.2_DBG.OBJ\\utf8.obj ..\\..\\..\\..\\dist\\W INNT5.2_DBG.OBJ\\lib\\libplc4.lib ..\\..\\..\\..\\dist\\WINNT5.2_DBG.OBJ\\lib\\l ibplds4.lib ..\\..\\..\\..\\dist\\WINNT5.2_DBG.OBJ\\lib\\libnspr4.lib WINNT5. 2_DBG.OBJ\\nssutil.res nssutil.def : error LNK2001: unresolved external symbol nss_mktemp WINNT5.2_DBG.OBJ/nssutil3.lib : fatal error LNK1120: 1 unresolved externals LINK : fatal error LNK1141: failure during build of exports file gmake[2]: *** [WINNT5.2_DBG.OBJ/nssutil3.dll] Error 1141 gmake[2]: Leaving directory `C:/NSS/tip2/mozilla/security/nss/lib/util' gmake[1]: *** [libs] Error 2 gmake[1]: Leaving directory `C:/NSS/tip2/mozilla/security/nss/lib' gmake: *** [libs] Error 2
Oops, nevermind, it is in your patch, in config.mk . I'm using VC6 to build, which we need to continue to support. I will try with VC8 since the article was about VC8. I don't know if VC6 supports this. I doubt OS/2 has anything this sophisticated for forwarding.
Same build issue with VC8 and its linker.
Comment on attachment 270101 [details] [diff] [review] Build nssutil as a shared library. There is a build issue on Windows with the export of mktemp . It is implemented in nssinit (libnss) but you try to export it from libutil. Once I removed this export, I ran into other issues linking softokn3 lowkey.obj : error LNK2001: unresolved external symbol _SEC_BitStringTemplate lowkey.obj : error LNK2001: unresolved external symbol _SEC_ObjectIDTemplate lowkey.obj : error LNK2001: unresolved external symbol _SECOID_AlgorithmIDTempla te sftkdb.obj : error LNK2001: unresolved external symbol _SECOID_AlgorithmIDTempla te lowkey.obj : error LNK2001: unresolved external symbol _SEC_AnyTemplate I think we need the NSS_Get_ function for these 4 templates.
Attachment #270101 - Flags: review-
I am working on fixing the patch for Windows. Once I fix it, I will try to check for binary compatibility.
Attached patch updated patch (obsolete) — — Splinter Review
This consolidates the previous 2 attachments. It fixes the build for the libs on Windows. The build for cmd is still broken in shlibsign . shlibsign tries to load "nssutil.dll" which doesn't exist.
Attachment #270101 - Attachment is obsolete: true
Attachment #270103 - Attachment is obsolete: true
Comment on attachment 270121 [details] [diff] [review] updated patch The failure to build was due to a typo in nssutil.def - the LIBRARY statement needed to say "nssutil3" rather than "nssutil" . This fixes the build error.
Attachment #270121 - Attachment is obsolete: true
Assignee: nobody → julien.pierre.boogz
Attached patch Fix build problems (obsolete) — — Splinter Review
I tested this on Windows only so far. I still need to test binary compatibility. Would appreciate if you could test that on Linux. I didn't use the forwarder feature in the DEF file. That isn't really necessary however - the forwarders are created automatically by the linker by virtue of all the util symbols being in nss.def . The forwarder statements are only necessary if we want to make the symbols private. And they will cause more work since they aren't supported on all platforms. I think the patch is good enough as-is . I will test at home on OS/2 as wlel.
Attachment #271952 - Flags: review?(rrelyea)
I'll attach the current .def files I have. BTW, there are additional template symbols exported in util that currently only nss3.dll uses. bob
Attached file NSS .def file with forwarding... (obsolete) —
Attached file nssutil.def updated (obsolete) —
This contains an updated nssutil.def that adds template accesssors. Use requires some code changes, so you can take this a just an example. NOTE: a couple of the templates that are exported are DER Templates, not ASN1. bob
Comment on attachment 271952 [details] [diff] [review] Fix build problems Unfortunately, I ran into build problems on Solaris, and runtime issues on Windows. The only platform on which this patch produces a build that allows all.sh to succeed is Linux .
Attachment #271952 - Attachment is obsolete: true
Attachment #271952 - Flags: review?(rrelyea)
It appears that the Solaris linker is not re-exporting the symbols from nssutil3.so by default. I haven't been able to figure out how to make it do that by default .
Attachment #272031 - Attachment is obsolete: true
Attachment #272032 - Attachment is obsolete: true
Attachment #272116 - Attachment is patch: false
I can get past the problem on Solaris by changing the linker option from -z defs to -z nodefs . But there are still warnings and this is not the proper solution because dependencies are not checked at build time. On Linux -z defs is used too but doesn't cause this problem.
Let's try removing the symbols then for solaris. This should work as long as solaris doesn't 'strongly bind' the symbols in the source. The hope would be that loading libnss3.so will automatically load libnssutil3.so and thus satisfy the unresolved symbols. bob
Bob, It appears that Solaris doesn't do the strong binding of symbols in the source program/library. I removed all those symbols from nss.def and was able to run all.sh from a regular tip build against the new libraries using libnssutil3.so . How should we handle this difference at build time in nss.def ? HP-UX still needs to be tested as well as AIX. There is also a crasher bug on Windows with all.sh that I have been trying to hunt down with the patch.
sounds like the Solaris linker command may be missing a certain command line option, (or may have a wrong option).
We should check HP-UX, AIX and Linux without the symbols. If the are fine, we should just have the Windows redirects in. I can check out the Linux side.
Nelson, What Solaris linker option do you think we are missing ? The strong binding option ? I am not sure if it exists. If it does, it's possible that some existing programs that are already linked wouldn't run against the new binaries. I'll try to have Christophe check that out. Bob, I will test HP-UX and AIX today. Please test Linux.
It appears that the tools we build on HP-UX also don't have the symbols strongly bound to a particular library - ie. if I remove them from nss.def like I did on Solaris and put them in nssutil.def, the old tools still run against the new libraries. attachment 272032 [details] also works on HP-UX without removing the symbols, so apparently the linker must be creating stubs, but I'm not certain of that because the nm output is ambiguous - the output for nm libnss3.so for the PORT functions for example is exactly the same whether I build with the currently-checked in nss.def, with attachment 272032 [details], or with that attachment modified to remove the symbols that are in nssutil.def . Here is one example : PORT_ZFree | |undef |code | PORT_ZFree | 417212|uext |stub |
AIX seems to be fine with the patch too ! So the ones that have problems are Solaris and Windows.
Please ignore my comment 38. I didn't realize that comment 37 was saying that the present state of things is a desirable state. <blush>
I believe the linker option to do strong binding on Solaris is -B direct . We are not using it in coreconf. But it's possible some of our applications are. We have no control over that. I will try to build tools with -B direct to verify that theory. Since I was unable to get forwarders to work on Solaris, I would expect binary compatibility to be broken for that case.
Bob, It appears that no forwarder entries are actually being created in libnss3.so to the libnssutil3.so symbols. Ie. the old symbols are no longer exported from it. I think this could cause problems if an application is strongly bound (ie. pulling the util symbol directly from libnss3.so) . But I am not very familiar with the runtime loader. It seems that right now the symbols get satisfied anyway because libnss3.so pulls in libnssutil3.so, even though the symbols don't get re-exported from libnss3.so . The Solaris linker is just the only one that complains about the problem and emits an error, but I think all the Unix platforms actually have the same problem. I examined the libnss3.so on Linux and certainly it has the issue.
Julien, This is how I would expect it to work. I was depending on the fact that nss3.so no longer had any entries, but the symbols would be automatically picked up by nssutil3 because nss3.so will pull nssutil3 in. The question is does this always work, are is there cases where the OS will complain because it 'knows' the symbol was supposed to be resolved by nss3.so. BTW it's the loader I care about, not the linker. So are you saying that this assumption is wrong for Sun, or wrong under certain conditions (and can we quantify what those conditions are). The alternative is a log uglier: we will need to replicate the data symbols, and we would have to hand forward the functions (stub functions in nss3 that call util3, and some how make sure the don't loop). bob
Bob, If you were depending on that, why didn't you just remove all the util symbols from nss.def ? What's the point of them being there if not to create forwarding entries ? I think on Linux the linker just ignores these entries altogether, and on Solaris it complains. I know the alternative is ugly, but this isn't pretty getting it to run everywhere either. One really needs specific knowledge of all the runtime loaders on all the different platforms. I am discussing the issue with our Solaris linker/loader guru, but I just don't know enough about the others. We may end up just having to check in what we have and test it with the existing applications we care about to see if just removing the util symbols from libnss3 and having libnss3 pull libnssutil3 works on all the unix platforms. Unless strong binding exists and has been done on those platforms, I would expect it to be OK.
Blocks: 389343
Unix doesn't have strong binding, otherwise LD_PRELOAD would not work. (For linking and loading, Mac OS X should not be treated as a Unix platform and needs to be separately tested.)
Wan-Teh, There is actually some strong binding in Solaris. To quote Rod Evans once again : "There's a model we use for per-symbol filtering. It's been employed in a number of our system libraries to do what I think you want - allow symbol implementations to move from one library to another, but preserve existing interfaces, or to consolidate multiple implementations to one implementation. For example, libc used to contain math functions that were a duplicate of the same functions in libm - maintaining consistency was a pain. So, we now point libc at the libm implementation - it looks like libc offers the symbols, but at runtime your are redirected to the one true implementation in libm: chaz 983. chaz 983. elfdump -y /lib/libc.so.1 | fgrep libm.so.2 [90] F [0] libm.so.2 _nextafter [156] F [0] libm.so.2 frexp [223] F [0] libm.so.2 _modf [292] F [0] libm.so.2 _modff ..... You can read about this technique in: http://docs.sun.com/app/docs/doc/819-0690/6n33n7f5q?a=view scroll down to Generating Standard Filters and look at the 4th example that uses a mapfile. The libc indirection I explained above would be brought about by building libc with a mapfile something like: _nextafter = FUNCTION FILTER libm.so.2; Note, this per-symbol filtering was added in Solaris 9 9/04. " This is not available to us yet since we must support Solaris 8+ with one set of binaries. There are also direct bindings coming up : http://opensolaris.org/os/community/tools/linker/LLM.pdf
I figured out what the crasher issue is for Windows. It is related to the old DER encoder. It doesn't have the ability to have subtemplates that cross DLLs, like the current encoder, which has the SEC_ASN1_XTRN and SEC_ASN1_SUB macros. There was only one instance where this was actually an issue : in secsign.c, the CERTSignedDataTemplate template in nss3.dll was using the SECAlgorithmIDTemplate from nssutil3.dll as a subtemplate. The result was just a garbage subtemplate causing a crash. There was only one other use of SECAlgorithmIDTemplate accross NSS, which was the SGNDigestInfoTemplate including it in nssutil3 . So, for this one case, I decided to make 2 static copies and make the symbol private (it was never previously public). This gets me past the crash in cert.sh .
Attached patch Updated patch (obsolete) — — Splinter Review
This patch builds on all platform. all.sh passes on Windows. It removes the util symbols from libnss3 and moves them to libnssutil3 . On Windows only, there are forwarder symbols in libnss3 in order to preserve binary compatibility. I am running the QA on all the platforms first and mixing old apps with the new libs to make sure everything is fine. I expect to ask for review on this very soon.
Attachment #272116 - Attachment is obsolete: true
Comment on attachment 275065 [details] [diff] [review] Updated patch At the end of the new nssutil.def file, these look like junk: >+;+# 0000ca8d T nss_InitMonitor >+;+# 0001fca0 B port_allocFailures >+;+# 000113e8 T sec_asn1d_uinteger ... You exported a few symbols that begin with a lowercase prefix. Can that be avoided? +__nss_InitLock; +;;sgn_DigestInfoTemplate DATA ; +nss_InitLock; +secoid_Init;
Bad news. Unfortunately, further testing revealed that AIX had strong bindings too , when I tried all.sh from a virgin tree against one with the patch : exec(): 0509-036 Cannot load program certutil because of the following errors: 0509-130 Symbol resolution failed for certutil because: 0509-136 Symbol SEC_GeneralizedTimeTemplate (number 63) is not exported from dependent module /h/monstre.red.iplanet.com/export/home/julien/nss/virgin_tip/mozilla/dist/AIX5.1_DBG.OBJ/lib/libnss3.so. 0509-136 Symbol SEC_ObjectIDTemplate (number 64) is not exported from dependent module /h/monstre.red.iplanet.com/export/home/julien/nss/virgin_tip/mozilla/dist/AI This is not necessarily a problem because I don't believe we ship products separate from shared libs on AIX, but it is less than ideal. Much more of a problem is that my testing on Linux shows problems too, on both RHEL3 and RHEL4 : ./certutil: relocation error: ./certutil: symbol SEC_ObjectIDTemplate, version NSS_3.2 not defined in file libnss3.so with link time reference This is when trying to run certutil from a virgin tree against the libnss3.so with the patch. It may be possible to fix this by playing with section names in nssutil.def.
No longer blocks: 389343
Attached patch Working patch (obsolete) — — Splinter Review
After much pain, this seems to be working. It builds and passes all.sh on Windows. It builds on Solaris, and my all.sh is still running. Please review. I know you suggested to rename the Moved_ prefix to a suffix before. I am open to doing that. But I am sure you will find other things in your review anyway.
Attachment #275065 - Attachment is obsolete: true
Attachment #277846 - Flags: review?
Comment on attachment 277846 [details] [diff] [review] Working patch "Util" would be a better prefix than "Moved".
I requested that the "Moved" word be a suffix, rather than a prefix.
Nelson, I know what you requested, I just didn't think it was critical to this review. I will change the name and prefix/suffix in my next update, after the rest of the patch is reviewed. I am perfectly fine with either a _Moved suffix, or a Util prefix as Wan-Teh suggested. FYI, the patch has passed QA on Solaris as well.
Julian, one thing I would like to see is the ability to turn off the whole rename for a platform. We know at least our Linux platform can handle Type 1 util move without a problem. Also, it looks like the way it's coded, newly compiled apps will still go through nss3.... bob
Bob, I haven't gotten type 1 to work with Linux. I tried RHEL3 and RHEL4. Which one did you try ? Try the following. 1) build a virgin tip 2) build another tree with attachment 275065 [details] [diff] [review] (this is type 1) 3) run all.sh from each tree separately. They will both pass 4) copy all files from type1/mozilla/dist/Linux/lib to virgin/mozilla/dist/Linux/lib 5) run virgin/mozilla/security/nss/tests/all.sh . It will fail because programs don't run. At least it did for me on Linux and AIX. As for newly compiled apps : if they don't make any build changes, they will go through nss3 by default. If they add nssutil3 to their link line, and define USE_UTIL_DIRECTLY in their C code, they will go through libnssutil3 directly.
OK, I'll retest my linux builds. I thought I was working. (RHEL5 & RHEL4). bob
Good news - attachment 277846 [details] [diff] [review] (approach 2) has passed all.sh on all our platforms - Solaris, Linux, Windows, HP-UX, AIX . The binary compatibility test with all.sh from a virgin tree against patched shared libraries also passed on all those platforms. There were 2 lines about macro redefinitions that I had to change for the patch to build on AIX. It built unchanged everywhere else. I will update the patch and also changed the prefix to a suffix.
Attachment #277846 - Flags: review? → review?(nelson)
Comment on attachment 277846 [details] [diff] [review] Working patch I emailed my review comments to Julien. It's mostly good, but there are a few things that need to be changed.
Attachment #277846 - Flags: review?(nelson) → review-
Attached patch completed patch (obsolete) — — Splinter Review
I made many changes in this patch, based in part on Nelson and Bob's comments, and in other parts on cleanup : 1) Merge for polcyxtn.c 2) Merge for legacydb . Had to fix nss_InitLock mess . Rename it only once, in utilrenam.h, and not in nssrenam.h anymore. 3) Changed Moved_ prefix to _Moved suffix 4) Added comment to coreconf/config.mk about USE_UTIL_DIRECTLY 5) Moved 2nd static copy of SECAlgorithmIDTemplate from templates.c to secsign.c . This is an old DER template 6) change include blapi.h in util/pqgutil.h to blapit.h, since we only use the PQGParams structure. 7) CERT_ValidityTemplate moved to certdb.c 8) secasn1d.c - #if 0 dead templates 9) remove templates from secdert.h that can't work accross DLLs 10) utilwrap.c : removed everything that wasn't in nss.def 11) nssutil.def : recreated from scratch, by looking at unresolved externals from other libraries during the build 12) sha-fast-amd-64.s : renamed PORT symbols that were being called from assembler source 13) licenses for new file - changed to 2007 and Sun Microsystems, Inc. as initial contributor 14) don't patch dead lib/asn1 sources
Attachment #277846 - Attachment is obsolete: true
Attachment #282628 - Flags: review?(nelson)
Comment on attachment 282628 [details] [diff] [review] completed patch This is VERY close, but I found two problems, one of which is mostly cosmetic, but the other makes me suspect this patch has not been built or tested on Windows. 1. In nss/lib/certdb/certdb.c we find: >+static const SEC_ASN1Template CERT_ValidityTemplate[] = { >+ { SEC_ASN1_SEQUENCE, >+ 0, NULL, sizeof(CERTValidity) }, >+ { SEC_ASN1_INLINE, >+ offsetof(CERTValidity,notBefore), CERT_TimeChoiceTemplate, 0 }, >+ { SEC_ASN1_INLINE, >+ offsetof(CERTValidity,notAfter), CERT_TimeChoiceTemplate, 0 }, >+ { 0 } >+}; >- { SEC_ASN1_INLINE, >+ { SEC_ASN1_INLINE | SEC_ASN1_XTRN, > offsetof(CERTCertificate,validity), >- CERT_ValidityTemplate }, >+ SEC_ASN1_SUB(CERT_ValidityTemplate) }, That SEC_ASN1_SUB will probably create a compilation error, because it will reference a symbol, p_NSS_Get_CERT_ValidityTemplate, which (I believe) does not exist. 2. in the new file nss/lib/util/nssutil.def the symbols do not appear to all be in ASCII sorting order. For example, we see: >+PORT_GetError_Moved; >+__nss_InitLock_Moved; >+ATOB_AsciiToData_Moved; and we see function name symbols that start with NSS both in the middle of the file and at the end. Also please change this comment (and any others like it) >+;+# Don't export these DATA symbols on Windows because they don't work right. to say something like "because data symbols cannot be used as initializers for data address items on Windows. Use the NSS_Get_ functions instead." Finally, maybe new code shouldn't be named after Netscape. >+ * The Original Code is the Netscape security libraries. In the next patch, please only include the files that are different from this patch. I don't want to review any files again if they haven't been changed since this monster patch. Thanks.
Attachment #282628 - Flags: review?(nelson) → review-
Attached patch Implement feedback from Nelson (obsolete) — — Splinter Review
1) in all new files, I changed the name of the initial product being changed from "Netscape security libraries" to "Network Security Services libraries" 2) in certdb.c, I fixed CERT_ValidityTemplate and the CERT_TimeChoiceTemplate it was referencing to resolve a build issue on Windows 3) in nssutil.def, I moved PORT_GetError . I did not otherwise switch the order of symbols. All the functions are at the beginning, in alphabetical order. They are followed by DATA symbols and their accessor functions. This is very similar to the initial checkin for nss.def . 4) in nss.def and nssutil.def, I changed the comment about the DATA symbols 5) all other files are unchanged from attachment 282628 [details] [diff] [review]
Attachment #282628 - Attachment is obsolete: true
Attachment #283309 - Flags: review?(nelson)
Comment on attachment 283309 [details] [diff] [review] Implement feedback from Nelson utilrenam.h should be renamed utilrename.h or utlrenam.h. nssrenam.h was named that way to conform to the 8.3 naming convention. If the 8.3 convention isn't necessary any more, it's better to use utilrename.h. Renaming these functions means one may need to know about the renaming to set the breakpoints in a debugger. I hope we can think of a suffix that looks nicer than _Moved. How about _Util? nss/lib/util/templates.c shouldn't need to include "utilrenam.h".
Wan-Teh, 1) The 8.3 convention only applies to DLL targets because of OS/2, but for all other files long names are OK. I will use utilrename.h . 2) _Util is fine with me and with Nelson too, so I will make that change as well. 3) As far as templates.c, it does need to include utilrename.h . That's because templates.c is compiled twice, once as part of libnssutil, which exports the renamed symbols, and once as included in utilwrap.c as part of libnss, which exports the original symbol names . In the first case, templates.c is compiled standalone, and it includes utilrename.h in order to compile and export the renamed symbols. In the second case, utilwrap.c includes utilrename.h first - by including other NSS headers - , then has a whole bunch of #undef to prevent the renaming from actually happening, then it includes templates.c . Even though templates.c includes utilrenam.h as well, since it has been previously #included by utilwrap.c, the macro _LIBUTIL_H_ is already defined, and so the template symbols are not redefined a second time, and therefore utilwrap.c includes the templates in templates.c with their original symbol names. 4) I found another bug in certdb.c . The CERT_CertificateTemplate shouldn't have SEC_ASN1_XTRN when including CERT_ValidityTemplate . This breaks the runtime on Windows only. My next patch will have this 1-line fix. I am still running some tests, so I will hold off attaching it until they all pass.
This has all the changes described in comment 67, which are all cosmetic except for Windows plus a build fix for AIX in certdb.c where some semicolons after SEC_ASN1_MKSUB statements and a static const discrepancy between the declaration and implementation of CERT_ValidityTemplate caused a build failure. I have successfully run the QA with the previous patch, attachment 283309 [details] [diff] [review] on HP-UX 32/64, Linux 32/64, Solaris Sparc v8/v9, Solaris x86/x64, for debug and optimized. That's 44 successful QA runs. The new patch is only cosmetic for those platforms, so I am not going to rerun the QA with it due to the time would add (1-2 days). I have also successfully run the build QA with the AIX-specific portions of the changes in this patch. I have built this new patch on Windows successfully, but am still running the QA on it. I have every reason to believe that it will pass however. My last run was a success except for some signtool errors that were due to my having rebuilt only the shared libraries but not relinked signtool to pick up the change in comment 67.4. I now need to run the binary compatibility tests for all platforms. I will rebuild all platforms with this updated patch to check for build issues, and run virgin tools against the new shared libraries. It is expected to take one more day to get the results due to a machine shortage, slow machines, and a very annoying PR_Bind issue that causes the QA to have to be restarted several times (bug 348198).
Attachment #283650 - Flags: review?(nelson)
- Sigh, there was one more problem in the patch in the assembly file for Solaris AMD64. My global search/replace from _Moved to _Util missed that .s file . - The QA has passed on Windows as well according to results.html . However, I see this at the end of the run. This doesn't look quite Kosher to me. Looks like failures, even . Bob, Slavo, can you comment ? ocsp.sh: Legacy to shared Library update =============================== alicedir upgrading db alicedir Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. bobdir upgrading db bobdir Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. CA upgrading db CA Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. cert_extensions upgrading db cert_extensions Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. client upgrading db client Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. clientCA upgrading db clientCA Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. dave upgrading db dave Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. eccurves upgrading db eccurves Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. eve upgrading db eve Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. ext_client upgrading db ext_client Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. ext_server upgrading db ext_server Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. SDR upgrading db SDR Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. server upgrading db server Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. serverCA upgrading db serverCA Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. tools/copydir upgrading db tools/copydir Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. upgrading db fips Generating key. This may take a few moments... certutil: unable to generate key(s) : An I/O error occurred during security authorization. FIPS_PUB_140_Test_Certificate ,,
Comment on attachment 283650 [details] [diff] [review] Incorporate Wan-Teh's feedback. Also fix Windows and AIX issues Deleting review request. The QA didn't pass on windows.
Attachment #283650 - Attachment is obsolete: true
Attachment #283650 - Flags: review?(nelson)
Comment on attachment 283309 [details] [diff] [review] Implement feedback from Nelson Deleting review request on older patch.
Attachment #283309 - Attachment is obsolete: true
Attachment #283309 - Flags: review?(nelson)
This patch changes 3 things : 1) fix a crash in certutil with the upgrade DB code. This was due to a missing SEC_ASN1_XTRN in one of the templates I changed with SEC_ASN1_SUB() . This crash was seen on Windows only. I am not sure why results.html was all green. I will need to file a bug about that. The certutil result code was probably not checked, otherwise the problem should have been detected. I didn't see the crash earlier due to the MKS signal handling. 2) fix the build on Solaris AMD64 - change the assembler source to refer to _Util symbols instead of _Moved. 3) an attempt to fix a template in the ckfw/nssmkey module, which is not built by default. I don't know how to build it so I can't verify these changes. This would only affect Windows. All tests passed on windows with this patch, including binary compatibility tests, except for certutil due to SEC_StringToOID being moved. For that one, I had to copy over certutil.exe . After copying over certutil due to the 1 moved symbol, binary compatibility tests also passed with attachment 283650 [details] [diff] [review] on Solaris Sparc 32-bit/64-bit, Solaris x86, Linux 32-bit/64-bit, and HP-UX 32-bit/64-bit, AIX 32-bit and 64-bit, both debug and optimized. The tests also passed on Solaris x64 with the update from this patch. At this point, all.sh has passed on all the platforms, as well as binary compatibility tests, so I am requesting what should be the last review.
Attachment #284230 - Flags: review?(nelson)
Julien Unfortunately, bugzilla thinks that the latest patch patches a different set of files than the previous one, and so is unable to show me the differences between the two sets of patches. This means a FULL re-review. :-( I'm assuming that the newest patch patches one or two more files than the previous one did. If that is so, then if you could create two new patches, one that patches the exact same set of files as the previous patch, and a second patch that patches the file(s) not patched in the previous patch, that would speed the review IMMENSELY.
It may or may not be relevant that all the file path names in the latest patch start at in the "security" directory, but in the previous patch they all started in the "mozilla" directory (one level higher).
The last 4 patches are named: completed patch Implement feedback from Nelson Incorporate Wan-Teh's feedback. Also fix Windows and AIX issues Fix upgrade db crash problem, Solaris AMD64 build Compared to each of the 3 patches before it, the last patch includes these additional files: - complete patch (and also) - Implement feedback from Nelson nss/lib/ckfw/nssmkey/mobject.c nss/lib/util/utilrenam.h nss/lib/util/utilrename.h - Incorporate Wan-Teh's feedback. Also fix Windows and AIX issues nss/lib/ckfw/nssmkey/mobject.c
Nelson, I meant to do all diffs from mozilla/security . I didn't do that for 283650, but it was a one-off mistake The set of files changed in 283650 because utilrenam.h was renamed utilrename.h . In attachment 284230 [details] [diff] [review], the only new file should be nss/lib/ckfw/nssmkey/mobject.c . Please tell me what kind of diff would help you with the review. Do you want one from mozilla to match attachment 283650 [details] [diff] [review], or from mozilla/security to match all the others ? Also, I can omit some files and let you review them separately, I just need to know which ones you want me to do that. One thing that can help you with the review is to just download the latest patches and run the diff command. I think you will see that it works pretty well and it should not be hard to review since the changes are small.
Julien, if you would precede the number of the bug with three letters 'bug', anybody reading your comment could see the ztitle of bug 283650 by just hovering over it, or could follow the link. Thanks for your work, I just want to learn. merci beaucoup en avant herman
Hermann, Actually in this case, I was referring to attachment 283650 [details] [diff] [review], not to bug 283650 .
This patch is not for review. It is to be compared with bugzilla's interdiff feature against attachment 283309 [details] [diff] [review] ("implement feedback from Nelson"). It contains the same code as attachment 284230 [details] [diff] [review], which is for review, except : 1) the change to mozilla/security/nss/lib/ckfw/nssmkey/mobject.c is omitted 2) mozilla/security/nss/lib/util/utilrenam.h still has that name, even though it was actually renamed to utilrename.h This is to get around the limitations of bugzilla's interdiff.
Julien, thanks for trying. Now, Interdiff complains: "Warning: this difference between two patches may be inaccurate due to a limitation in Bugzilla when comparing patches made against different revisions." And, yes, the differences shown are utter nonsense. See, for example, this: <https://bugzilla.mozilla.org/attachment.cgi?oldid=283309&action=interdiff&newid=284410&headers=1#mozilla/security/nss/lib/certhigh/ocsp.c_sec3> Look at the "hunks" labeled lines 1082-1090 and lines 1094-1102. They make it appear that you removed XTRN and _SUB macros in one place, and added them in the other, when in fact your latest patch adds them in two places. Sigh.
Nelson, I don't think there is anything more I can do to make interdiff work. It is a broken tool. We can't change the job we have to do in order to fit the broken tool. I suggest that you just download the last patch you reviewed and this one, and diff them. The only changes are semicolons, symbol renames, and only a couple of SEC_ASN1_XTRN templates that you could look up the context for manually if you want. It should be a short review.
Nelson, This diff of patches shows that everything that changed since your review is minor, namely : 1) some semicolons were removed for AIX 2) the suffix was changed from _Moved to _Util 3) static was removed from the definition of CERT_ValidityTemplate for AIX 4) SEC_ASN1_XTRN was 5 times in templates, as its absence was causing crashes on Windows, including once in nss/lib/ckfw/nssmkey/mobject.c which was previously an unpatched file 5) utilrenam.h was renamed utilrename.h This should help you speed up the review of attachment 284230 [details] [diff] [review] .
Attachment #284410 - Attachment is obsolete: true
Comment on attachment 284545 [details] diff of attachment 283309 [details] [diff] [review] (implement feedback from Nelson) and attachment 284230 [details] [diff] [review] (fix upgrade DB crash problem) Diffs of diffs are ok for differences like this: 1589c1606 < + return PORT_Strdup_Moved(str); --- > + return PORT_Strdup_Util(str); But are quite worthless for patches like this: 2531c2549 < { SEC_ASN1_INLINE, offsetof(nsspkcs5V2PBEParameter, keyParams), --- > - { SEC_ASN1_INLINE, offsetof(nsspkcs5V2PBEParameter, keyParams), 2533,2534c2551 < + SEC_ASN1_SUB(SECOID_AlgorithmIDTemplate) }, < { SEC_ASN1_INLINE, offsetof(nsspkcs5V2PBEParameter, algParams), --- > - { SEC_ASN1_INLINE, offsetof(nsspkcs5V2PBEParameter, algParams), 2535a2553,2555 > + { SEC_ASN1_INLINE | SEC_ASN1_XTRN, offsetof(nsspkcs5V2PBEParameter, keyParams), > + SEC_ASN1_SUB(SECOID_AlgorithmIDTemplate) }, > + { SEC_ASN1_INLINE | SEC_ASN1_XTRN, offsetof(nsspkcs5V2PBEParameter, algParams), There are certain files whose changes I cannot review based on the diffs of diffs. They include: nss/lib/certdb/certdb.c nss/lib/softoken/lowpbe.c I'll find some way to review them today.
Comment on attachment 284230 [details] [diff] [review] Fix upgrade db crash problem, Solaris AMD64 build r=nelson, provided you make a few simple changes shown below. The insertion of SEC_ASN1_SUB, SEC_ASN1_GET and/or SEC_ASN1_XTRN caused quite a few lines of c source to extend well beyond 80 columns. Those lines are shown below. Please fold them. Note that I gave a pass to utilrename.h. It had quite a few lines that were too long, but due to its special nature, I just ignored those. +++ nss/lib/certdb/certdb.c +>>>>>>>>>>ffsetof(CERTValidity,notBefore), SEC_ASN1_SUB(CERT_TimeChoiceTemplate), 0 }, +>>>>>>>>>>ffsetof(CERTValidity,notAfter), SEC_ASN1_SUB(CERT_TimeChoiceTemplate), 0 }, +++ nss/lib/certdb/certv3.c +>>>>>>>>>> SEC_ASN1_GET(SEC_IA5StringTemplate), &baseitem); +++ nss/lib/certhigh/crlv2.c +>>>>>>>>>>EC_QuickDERDecodeItem(arena, &tmpItem, SEC_ASN1_GET(SEC_EnumeratedTemplate), +++ nss/lib/certhigh/ocsp.c +>>>>>>>>>> SEC_ASN1_GET(SEC_SequenceOfObjectIDTemplate)); +++ nss/lib/cryptohi/secsign.c +>>>>>>>>>>= DER_Encode(arena, &digder, SEC_ASN1_GET(SGNDigestInfoTemplate), di); +>>>>>>>>>>= DER_Encode(arena, &digder, SEC_ASN1_GET(SGNDigestInfoTemplate), di); +++ nss/lib/pk11wrap/pk11pk12.c +>>>>>>>>>>ASN1_INLINE | SEC_ASN1_XTRN, offsetof(SECKEYPrivateKeyInfo,algorithm), +++ nss/lib/softoken/lowkey.c +>>>>>>>>>>ASN1_SET_OF | SEC_ASN1_XTRN , offsetof(NSSLOWKEYAttribute, attrValue), +++ nss/lib/softoken/lowpbe.c +>>>>>>>>>>ASN1_INLINE | SEC_ASN1_XTRN, offsetof(nsspkcs5V2PBEParameter, keyParams), +>>>>>>>>>> SEC_ASN1_SUB(SECOID_AlgorithmIDTemplate) }, +>>>>>>>>>>ASN1_INLINE | SEC_ASN1_XTRN, offsetof(nsspkcs5V2PBEParameter, algParams), +>>>>>>>>>> SEC_ASN1_SUB(SECOID_AlgorithmIDTemplate) }, +>>>>>>>>>> SEC_ASN1_SUB(SECOID_AlgorithmIDTemplate) }, +++ nss/lib/softoken/legacydb/lowcert.c +>>>>>>>>>>ASN1_INLINE | SEC_ASN1_XTRN, offsetof(NSSLOWCERTSubjectPublicKeyInfo,algorithm),
Attachment #284230 - Flags: review?(nelson) → review+
Some general comments from me about comparing patch revisions. I mentioned some of them during today's call, but I think it's a good idea to write them down. I don't use Bugzilla's Interdiff feature a lot. Actually I just learned about it recently. And I think it's failing too often. I'm using tkdiff, which is based on tcl/tk, so it's quite portable: http://sourceforge.net/projects/tkdiff/ When you make a new patch revision, look at your previous patch. Learn where you previously ran the "cvs diff" command, and go to the same directory. As Nelson noted before, this rule was not followed between patch revisions. But I was easily able to use the attached patches to produce new patches that are based on the same directory level. The old patches still applied fine to the most recent tree. I encourage you to work with the "patch --dry-run" option. I use that all the time. Whenever I play with a patch, I first do a dry run to see whether it really applies. It will give me a success or failure report without making any changes. So, I took Julien's earlier patch (attachment 283650 [details] [diff] [review]). # cd mozilla # cat patch-283650 | patch --dry-run -p0 applies fine Next, I took Julien's newer patch (attachment 284230 [details] [diff] [review]) # cd mozilla/security # cat patch-284230 | patch --dry-run -p0 applies fine # cd mozilla # cvs diff -u security/coreconf security/nss | tee patch2-284230 I looked at the file sizes and realized, some changes are missing, so obviously Julien's patch has new files. No problem. Let's add them automatically. I can recommend another helpful command line utility which is part of the "patchutils" package. http://cyberelk.net/tim/software/patchutils/ # lsdiff patch-284230 This gives you a listing of all filenames in the patch, so I did # cd mozilla/security # cat patch-284230 | lsdiff looks good, so # cat patch-284230 | lsdiff | xargs cvs add (I'm using "bash" and this runs "cvs add" with all filenames on the command line) This gives me some output that most files are already added to cvs, but it will add the missing ones. So, I'm repeating the previous thing, with the -N option for new files: # cd mozilla # cvs diff -u -N security/coreconf security/nss | tee patch2-284230 The new patch file uses equivalent filenames than the earlier patch. I could now attach patch2-284320 and it would probably work with bugzilla's interdiff. Now, I run # tkdiff patch-283650 patch2-284230 and resize it to full screen. Most of the screen background is white, but it still lists a couple of changed lines per page (colored background), but you quickly learn that most lines are simply timestamp lines. tkdiff is nice, within each changed line, that uses a light background, it even gives you further highlighting. It uses a darker background color to further highlight the characters that changed. Identical characters have the lighter background color. While you tab down, you can easily distinguish those cvs-filename-timestamp lines and skip over them. Added Blocks are bright green on the right side, deleted blocks are bright red on the left side. I always use tkdiff to compare my new patch with my previous patch, because: - I want to remind myself what I changed - I want to ensure the patch is similar to the previous patch, to ease reviewing Actually, I normalized both patches to be at the highest level, so they use the full patch mozilla/security/.... I'll attach both normalized patches for testing, so we can see whether bugzilla is able to interdiff them.
Attached patch Test 1 — — Splinter Review
Normalized version of attachment 283650 [details] [diff] [review]
Attached patch Test 2 — — Splinter Review
Normalized version of attachment 284230 [details] [diff] [review].
Kai, I believe all my patches were made from mozilla/security, except for attachment 283650 [details] [diff] [review] . That was a one-off error. But the last patch reviewed by Nelson (verbally, in his office with me) was attachment 283309 [details] [diff] [review] . So, I wanted to show the changes since that one.
Looking at the interdiff between Test 1 and Test 2, I'm learning, although all both patches refer to the same filenames and same cvs file revisions, interdiff still makes mistakes. Actually, the interdiff output seems identical to what you get when you compare Julien's original files! Interdiff incorrectly claims that changes to file lowpbe.c are from file mobject.c. I guess that problem can not be fixed by us, because the first patch didn't contain any changes to file mobject.c yet. So the precondition required by bugzilla's interdiff "must use exactly the same list of files" cannot be fulfiled when your new patch has new changes to additional files. You would have to manually edit the new patch file with a text editor, move the new chunks to a separate file, and attach two patch files. I personally think that using tkdiff for comparing patch revisions is much easier.
Nelson, Thanks for the review. I fixed the indentation and committed the patch to the trunk, finally. Unfortunately, there were so many affected files that the cvs commit log didn't fit in my shell buffer. Here is what I was able to retrieve : new revision: 1.35; previous revision: 1.34 done Checking in nss/lib/certdb/polcyxtn.c; /cvsroot/mozilla/security/nss/lib/certdb/polcyxtn.c,v <-- polcyxtn.c new revision: 1.9; previous revision: 1.8 done Checking in nss/lib/certdb/secname.c; /cvsroot/mozilla/security/nss/lib/certdb/secname.c,v <-- secname.c new revision: 1.22; previous revision: 1.21 done Checking in nss/lib/certdb/xauthkid.c; /cvsroot/mozilla/security/nss/lib/certdb/xauthkid.c,v <-- xauthkid.c new revision: 1.6; previous revision: 1.5 done Checking in nss/lib/certdb/xconst.c; /cvsroot/mozilla/security/nss/lib/certdb/xconst.c,v <-- xconst.c new revision: 1.11; previous revision: 1.10 done Checking in nss/lib/certhigh/certreq.c; /cvsroot/mozilla/security/nss/lib/certhigh/certreq.c,v <-- certreq.c new revision: 1.7; previous revision: 1.6 done Checking in nss/lib/certhigh/crlv2.c; /cvsroot/mozilla/security/nss/lib/certhigh/crlv2.c,v <-- crlv2.c new revision: 1.6; previous revision: 1.5 done Checking in nss/lib/certhigh/ocsp.c; /cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v <-- ocsp.c new revision: 1.44; previous revision: 1.43 done Checking in nss/lib/certhigh/xcrldist.c; /cvsroot/mozilla/security/nss/lib/certhigh/xcrldist.c,v <-- xcrldist.c new revision: 1.5; previous revision: 1.4 done Checking in nss/lib/ckfw/nssmkey/mobject.c; /cvsroot/mozilla/security/nss/lib/ckfw/nssmkey/mobject.c,v <-- mobject.c new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/cryptohi/seckey.c; /cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v <-- seckey.c new revision: 1.45; previous revision: 1.44 done Checking in nss/lib/cryptohi/secsign.c; /cvsroot/mozilla/security/nss/lib/cryptohi/secsign.c,v <-- secsign.c new revision: 1.19; previous revision: 1.18 done Checking in nss/lib/freebl/config.mk; /cvsroot/mozilla/security/nss/lib/freebl/config.mk,v <-- config.mk new revision: 1.19; previous revision: 1.18 done Checking in nss/lib/freebl/sha-fast-amd64-sun.s; /cvsroot/mozilla/security/nss/lib/freebl/sha-fast-amd64-sun.s,v <-- sha-fast-a md64-sun.s new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_ldaptemplates.c; /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_ldaptemplat es.c,v <-- pkix_pl_ldaptemplates.c new revision: 1.4; previous revision: 1.3 done Checking in nss/lib/nss/config.mk; /cvsroot/mozilla/security/nss/lib/nss/config.mk,v <-- config.mk new revision: 1.31; previous revision: 1.30 done Checking in nss/lib/nss/manifest.mn; /cvsroot/mozilla/security/nss/lib/nss/manifest.mn,v <-- manifest.mn new revision: 1.10; previous revision: 1.9 done Checking in nss/lib/nss/nss.def; /cvsroot/mozilla/security/nss/lib/nss/nss.def,v <-- nss.def new revision: 1.180; previous revision: 1.179 done Checking in nss/lib/nss/nssrenam.h; /cvsroot/mozilla/security/nss/lib/nss/nssrenam.h,v <-- nssrenam.h new revision: 1.4; previous revision: 1.3 done RCS file: /cvsroot/mozilla/security/nss/lib/nss/utilwrap.c,v done Checking in nss/lib/nss/utilwrap.c; /cvsroot/mozilla/security/nss/lib/nss/utilwrap.c,v <-- utilwrap.c initial revision: 1.1 done Checking in nss/lib/pk11wrap/pk11cert.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v <-- pk11cert.c new revision: 1.160; previous revision: 1.159 done Checking in nss/lib/pk11wrap/pk11mech.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11mech.c,v <-- pk11mech.c new revision: 1.8; previous revision: 1.7 done Checking in nss/lib/pk11wrap/pk11pk12.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pk12.c,v <-- pk11pk12.c new revision: 1.12; previous revision: 1.11 done Checking in nss/lib/pk11wrap/pk11sdr.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11sdr.c,v <-- pk11sdr.c new revision: 1.16; previous revision: 1.15 done Checking in nss/lib/smime/config.mk; /cvsroot/mozilla/security/nss/lib/smime/config.mk,v <-- config.mk new revision: 1.24; previous revision: 1.23 done Checking in nss/lib/softoken/config.mk; /cvsroot/mozilla/security/nss/lib/softoken/config.mk,v <-- config.mk new revision: 1.22; previous revision: 1.21 done Checking in nss/lib/softoken/fipstest.c; /cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v <-- fipstest.c new revision: 1.23; previous revision: 1.22 done Checking in nss/lib/softoken/lowkey.c; /cvsroot/mozilla/security/nss/lib/softoken/lowkey.c,v <-- lowkey.c new revision: 1.9; previous revision: 1.8 done Checking in nss/lib/softoken/lowpbe.c; /cvsroot/mozilla/security/nss/lib/softoken/lowpbe.c,v <-- lowpbe.c new revision: 1.20; previous revision: 1.19 done Checking in nss/lib/softoken/pkcs11c.c; /cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v <-- pkcs11c.c new revision: 1.96; previous revision: 1.95 done Checking in nss/lib/softoken/sftkpwd.c; /cvsroot/mozilla/security/nss/lib/softoken/sftkpwd.c,v <-- sftkpwd.c new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/softoken/legacydb/config.mk; /cvsroot/mozilla/security/nss/lib/softoken/legacydb/config.mk,v <-- config.mk new revision: 1.4; previous revision: 1.3 done Checking in nss/lib/softoken/legacydb/keydb.c; /cvsroot/mozilla/security/nss/lib/softoken/legacydb/keydb.c,v <-- keydb.c new revision: 1.6; previous revision: 1.5 done Checking in nss/lib/softoken/legacydb/lowcert.c; /cvsroot/mozilla/security/nss/lib/softoken/legacydb/lowcert.c,v <-- lowcert.c new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/softoken/legacydb/lowkey.c; /cvsroot/mozilla/security/nss/lib/softoken/legacydb/lowkey.c,v <-- lowkey.c new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/ssl/config.mk; /cvsroot/mozilla/security/nss/lib/ssl/config.mk,v <-- config.mk new revision: 1.23; previous revision: 1.22 done Checking in nss/lib/util/base64.h; /cvsroot/mozilla/security/nss/lib/util/base64.h,v <-- base64.h new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/util/ciferfam.h; /cvsroot/mozilla/security/nss/lib/util/ciferfam.h,v <-- ciferfam.h new revision: 1.4; previous revision: 1.3 done Checking in nss/lib/util/config.mk; /cvsroot/mozilla/security/nss/lib/util/config.mk,v <-- config.mk new revision: 1.4; previous revision: 1.3 done Checking in nss/lib/util/derenc.c; /cvsroot/mozilla/security/nss/lib/util/derenc.c,v <-- derenc.c new revision: 1.5; previous revision: 1.4 done Checking in nss/lib/util/manifest.mn; /cvsroot/mozilla/security/nss/lib/util/manifest.mn,v <-- manifest.mn new revision: 1.15; previous revision: 1.14 done Checking in nss/lib/util/nssb64.h; /cvsroot/mozilla/security/nss/lib/util/nssb64.h,v <-- nssb64.h new revision: 1.4; previous revision: 1.3 done Checking in nss/lib/util/nssb64t.h; /cvsroot/mozilla/security/nss/lib/util/nssb64t.h,v <-- nssb64t.h new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/util/nssilckt.h; /cvsroot/mozilla/security/nss/lib/util/nssilckt.h,v <-- nssilckt.h new revision: 1.6; previous revision: 1.5 done Checking in nss/lib/util/nssilock.h; /cvsroot/mozilla/security/nss/lib/util/nssilock.h,v <-- nssilock.h new revision: 1.5; previous revision: 1.4 done Checking in nss/lib/util/nsslocks.c; /cvsroot/mozilla/security/nss/lib/util/nsslocks.c,v <-- nsslocks.c new revision: 1.6; previous revision: 1.5 done Checking in nss/lib/util/nsslocks.h; /cvsroot/mozilla/security/nss/lib/util/nsslocks.h,v <-- nsslocks.h new revision: 1.4; previous revision: 1.3 done Checking in nss/lib/util/nssrwlk.h; /cvsroot/mozilla/security/nss/lib/util/nssrwlk.h,v <-- nssrwlk.h new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/util/nssrwlkt.h; /cvsroot/mozilla/security/nss/lib/util/nssrwlkt.h,v <-- nssrwlkt.h new revision: 1.4; previous revision: 1.3 done RCS file: /cvsroot/mozilla/security/nss/lib/util/nssutil.def,v done Checking in nss/lib/util/nssutil.def; /cvsroot/mozilla/security/nss/lib/util/nssutil.def,v <-- nssutil.def initial revision: 1.1 done RCS file: /cvsroot/mozilla/security/nss/lib/util/nssutil.rc,v done Checking in nss/lib/util/nssutil.rc; /cvsroot/mozilla/security/nss/lib/util/nssutil.rc,v <-- nssutil.rc initial revision: 1.1 done Checking in nss/lib/util/portreg.h; /cvsroot/mozilla/security/nss/lib/util/portreg.h,v <-- portreg.h new revision: 1.4; previous revision: 1.3 done Checking in nss/lib/util/pqgutil.h; /cvsroot/mozilla/security/nss/lib/util/pqgutil.h,v <-- pqgutil.h new revision: 1.5; previous revision: 1.4 done Checking in nss/lib/util/secalgid.c; /cvsroot/mozilla/security/nss/lib/util/secalgid.c,v <-- secalgid.c new revision: 1.6; previous revision: 1.5 done Checking in nss/lib/util/secasn1.h; /cvsroot/mozilla/security/nss/lib/util/secasn1.h,v <-- secasn1.h new revision: 1.15; previous revision: 1.14 done Checking in nss/lib/util/secasn1d.c; /cvsroot/mozilla/security/nss/lib/util/secasn1d.c,v <-- secasn1d.c new revision: 1.38; previous revision: 1.37 done Checking in nss/lib/util/secasn1t.h; /cvsroot/mozilla/security/nss/lib/util/secasn1t.h,v <-- secasn1t.h new revision: 1.10; previous revision: 1.9 done Checking in nss/lib/util/seccomon.h; /cvsroot/mozilla/security/nss/lib/util/seccomon.h,v <-- seccomon.h new revision: 1.7; previous revision: 1.6 done Checking in nss/lib/util/secder.h; /cvsroot/mozilla/security/nss/lib/util/secder.h,v <-- secder.h new revision: 1.11; previous revision: 1.10 done Checking in nss/lib/util/secdert.h; /cvsroot/mozilla/security/nss/lib/util/secdert.h,v <-- secdert.h new revision: 1.5; previous revision: 1.4 done Checking in nss/lib/util/secdig.c; /cvsroot/mozilla/security/nss/lib/util/secdig.c,v <-- secdig.c new revision: 1.8; previous revision: 1.7 done Checking in nss/lib/util/secdig.h; /cvsroot/mozilla/security/nss/lib/util/secdig.h,v <-- secdig.h new revision: 1.6; previous revision: 1.5 done Checking in nss/lib/util/secdigt.h; /cvsroot/mozilla/security/nss/lib/util/secdigt.h,v <-- secdigt.h new revision: 1.4; previous revision: 1.3 done Checking in nss/lib/util/secerr.h; /cvsroot/mozilla/security/nss/lib/util/secerr.h,v <-- secerr.h new revision: 1.21; previous revision: 1.20 done Checking in nss/lib/util/secitem.h; /cvsroot/mozilla/security/nss/lib/util/secitem.h,v <-- secitem.h new revision: 1.7; previous revision: 1.6 done Checking in nss/lib/util/secoid.h; /cvsroot/mozilla/security/nss/lib/util/secoid.h,v <-- secoid.h new revision: 1.8; previous revision: 1.7 done Checking in nss/lib/util/secoidt.h; /cvsroot/mozilla/security/nss/lib/util/secoidt.h,v <-- secoidt.h new revision: 1.26; previous revision: 1.25 done Checking in nss/lib/util/secplcy.h; /cvsroot/mozilla/security/nss/lib/util/secplcy.h,v <-- secplcy.h new revision: 1.3; previous revision: 1.2 done Checking in nss/lib/util/secport.h; /cvsroot/mozilla/security/nss/lib/util/secport.h,v <-- secport.h new revision: 1.14; previous revision: 1.13 done Checking in nss/lib/util/sectime.c; /cvsroot/mozilla/security/nss/lib/util/sectime.c,v <-- sectime.c new revision: 1.8; previous revision: 1.7 done RCS file: /cvsroot/mozilla/security/nss/lib/util/templates.c,v done Checking in nss/lib/util/templates.c; /cvsroot/mozilla/security/nss/lib/util/templates.c,v <-- templates.c initial revision: 1.1 done RCS file: /cvsroot/mozilla/security/nss/lib/util/utilrename.h,v done Checking in nss/lib/util/utilrename.h; /cvsroot/mozilla/security/nss/lib/util/utilrename.h,v <-- utilrename.h initial revision: 1.1 done I'm marking this bug as FIXED.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Kai, as I said in the call, for Interdiff to work the patches must be to EXACTLY the same set of files, and exactly the same versions. (I'm not sure if order of files is important or not)
I think those 2 limitations of interdiff are pretty severe. Re: same set of files, sometimes people request new file names to be changed, or omissions are found and new files need to be patched. Both happened here at the last minute. Sometimes, the tree moves during the development of the patch, and newly added files also need to be modified. This was the case with legacydb which was added since the first patch. Re: the same versions, it is not always possible, especially when the tree moves a lot. This bug took 2.5 years to fix, and over 3 months of active development. There weren't too many conflicts along away, but obviously there were some. My opinion is that we should either try to get interdiff fixed, or find better tools. I haven't tried tkdiff yet as described by Kai, but it sounds interesting. It would be great if this could be automated, maybe by a script, to make it easier for us. And even better if that script can be a CGI on b.m.o . Another approach would be to allow the large patches to go in earlier if they are reasonably OK, and keep the bug opened until minor objections are resolved by subsequent, smaller patches. I think that could have worked OK in this case. Just my 2 cents.
I noticed the tinderbox went red after my last checkin in the JSS build, which I did not test. This is because all the C code built with coreconf defines USE_UTIL_DIRECTLY, which renames symbols, and requires binaries to be linked with the new libnssutil3 shared libraries. One way to fix this would be not to compile JSS with USE_UTIL_DIRECTLY, but the proper way is to link it to nssutil3 to get to the implementation of the functions directly, rather than the wrappers which are in libnss3. I have tested this patch locally and it appears to fix the issue, but I am not sure if I built all of JSS since I don't have much experience with it. Note that I checked this in already in order to make the tinderbox go green. Checking in config.mk; /cvsroot/mozilla/security/jss/lib/config.mk,v <-- config.mk new revision: 1.23; previous revision: 1.22
Attachment #284575 - Flags: review?(glen.beasley)
Blocks: 399589
Julien, looks like this checkin may have caused bug 399589. Please investigate that.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #92) I agree that when a bug is actively being worked on, we should check in large patches that are good enough and address the remaining issues in subsequent patches.
No longer blocks: 399589
Comment on attachment 284575 [details] [diff] [review] Fix broken JSS build by linking to new nssutil3 shared lib r- You need to ifdef USE_UTIL_DIRECTLY the addition of libnssutil3
Christophe, Re: comment 96, My util patch was designed so that everything that uses coreconf from the NSS trunk should link with the libnssutil from the NSS trunk . Thus, there is no Makefile macro definition for USE_UTIL_DIRECTLY to test for. There is only a macro being passed to the C compiler (DEFINES += -DUSE_UTIL_DIRECTLY) defined in coreconf/config.mk. Unfortunately, JSS does not have a branch and the same JSS trunk sources need to be able to build either against either the NSS 3.11 branch coreconf, or against the trunk coreconf. I think you have to define a new build macro in your build script, and then test for it in the JSS Makefile. I think you or Glen should implement that part, since it's a JSS build problem. I only tried to fix it myself last thursday because the NSS/JSS trunk tinderbox got orange. I didn't know the NSS/JSS "branch" tinderbox would be affected by this. Nelson, Re: comment 94, bug 399589 was a problem in PSM. PSM was relying on an erroneous declaration of a private symbol in an NSS header, but had its own implementation for the symbol.
Attachment #284575 - Attachment is obsolete: true
Attachment #284575 - Flags: review?(glen.beasley)
I have opened bug 400094 for the JSS build issue. This bug is done for the NSS side.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Julien, what is the net effect of this work on code size for NSS? How does NSS code size compare, with and without this patch?
I haven't done measurements on the day of the checkin. I don't think we have the nightlies to look at anymore. The total varies per platform and compiler. For Linux x86 this accounted for a 100 KB code size reduction prior to other optimizations like changing -Os according to bug 389343 comment 39.
Julien, This big patch created a new warning that may indicate a real problem. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib/util/secdig.c&rev=1.8#54 After the definition of DERTemplate SGNDigestInfoTemplate[] this patch inserted: SEC_ASN1_CHOOSER_IMPLEMENT(SGNDigestInfoTemplate) But the template argument for that macro must be a SEC_ASN1Template, not a DERTemplate. My review didn't catch this discrepancy. :-/
Nelson, What's the exact warning you are seeing ? Is it on the IMPLEMENT, DECLARE or the GET macro ? I am aware that the SEC_ASN1_CHOOSER_IMPLEMENT macro and its DECLARE, GET, and SUB counterparts were designed for ASN.1 templates and not for DER templates. But in fact, they work for any data symbol, thanks to C's weak typing, which I admit I (intentionally) abused here, without your noticing in the code review. For several DER templates, I had to copy the definition, because they had cross-DLLs inlined DER templates. This isn't the case here with SGNDigestInfoTemplate . It has one inline, but it is defined right above in the same source, so there is no runtime issue. There are several ways to fix the warning. If the warning is on the three instances of SEC_ASN1_GET(SGNDigestInfoTemplate), I would suggest simply adding explicit casts to silence the compiler since this isn't causing a real problem.
The warning is: secdig.c(69) : warning C4133: 'return' : incompatible types - from 'DERTemplate [4]' to 'const SEC_ASN1Template *'
Attached patch Add typing to the macros (obsolete) — — Splinter Review
Attachment #286250 - Flags: review?(nelson)
Here's an alternative suggestion. What do you think of this? How about leaving the SEC_ASN1_CHOOSER_* macros alone, and writing some new DER_TEMPLATE_CHOOSER_* macros?
Nelson, I didn't want to add new macros specifically for DER templates because we are trying to get rid of DER templates. The new macros I added are generic and can be used for any type, so they might be useful in the future for cases other than this one.
reopening, so that the review request will show up on my radar. I don't see review requests for resolved bugs.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch alternative macro typing patch (obsolete) — — Splinter Review
Julien, I'd like to propose this alternative macro typing patch. Please review.
Attachment #287191 - Flags: review?(julien.pierre.boogz)
Comment on attachment 287191 [details] [diff] [review] alternative macro typing patch I like Nelson's patch better. Let's do it.
Attachment #287191 - Flags: review+
Comment on attachment 287191 [details] [diff] [review] alternative macro typing patch It would be better to rename these two macros DER_CHOOSER_DECLARE and DER_CHOOSER_IMPLEMENT and define them in secdert.h instead of secasn1t.h.
Comment on attachment 286250 [details] [diff] [review] Add typing to the macros I concur with Wan-Teh's comment 110.
Attachment #286250 - Flags: review?(nelson) → review-
Comment on attachment 287191 [details] [diff] [review] alternative macro typing patch obsoleting this patch, because I concur with Wan-Teh's comment 110.
Attachment #287191 - Attachment is obsolete: true
Attachment #287191 - Flags: review?(julien.pierre.boogz)
Arg! We should not export any DERTemplates. We have been trying to remove DERTemplates for years. We've managed to keep from exporting them to this point in NSS. That one template should be copied to where it is needed. Someday it should go away. bob
Comment on attachment 287191 [details] [diff] [review] alternative macro typing patch As I previously stated in comment 106, I didn't want to add macros specific to the DER templates, so I would object to this patch. I am fine with bob's recommendation in comment 113 to just copy this one template to the 2 places where it is being used. I will attach a patch that does that.
Attachment #287191 - Flags: superreview-
Comment on attachment 287595 [details] [diff] [review] Don't export SGNDigestInfoTemplate from libnssutil3. Copy it to cryptohi and softoken where used. (checked in) r=nelson
Attachment #287595 - Flags: review+
Comment on attachment 287595 [details] [diff] [review] Don't export SGNDigestInfoTemplate from libnssutil3. Copy it to cryptohi and softoken where used. (checked in) Thanks for the review, Nelson. I checked this in to the trunk. Checking in cryptohi/secsign.c; /cvsroot/mozilla/security/nss/lib/cryptohi/secsign.c,v <-- secsign.c new revision: 1.20; previous revision: 1.19 done Checking in softoken/pkcs11c.c; /cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v <-- pkcs11c.c new revision: 1.97; previous revision: 1.96 done Checking in util/nssutil.def; /cvsroot/mozilla/security/nss/lib/util/nssutil.def,v <-- nssutil.def new revision: 1.2; previous revision: 1.1 done Checking in util/secdig.c; /cvsroot/mozilla/security/nss/lib/util/secdig.c,v <-- secdig.c new revision: 1.9; previous revision: 1.8 done Checking in util/secdig.h; /cvsroot/mozilla/security/nss/lib/util/secdig.h,v <-- secdig.h new revision: 1.7; previous revision: 1.6 done
Attachment #287595 - Attachment description: Don't export SGNDigestInfoTemplate from libnssutil3. Copy it to cryptohi and softoken where uesd. → Don't export SGNDigestInfoTemplate from libnssutil3. Copy it to cryptohi and softoken where used. (checked in)
Attachment #287595 - Flags: review?(rrelyea)
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
This allows us to delete the DERTemplates altogether. There happens to be an unused function SGN_EncodeDigestInfo. So I export it from libnssutil3.so. Questions: 1. The idea of this patch seems obvious to me. Since you didn't suggest it, I am worried that I may have missed something. Does SEC_ASN1EncodeItem not guarantee that the output is encoded using DER? 2. SGN_EncodeDigestInfo returns SECItem*. This allows it to take a NULL input argument and allocate the result SECItem for the caller. We don't need this (because DER_Encode doesn't do this). Should I change SGN_EncodeDigestInfo to return SECStatus instead?
Attachment #288152 - Flags: superreview?(rrelyea)
Attachment #288152 - Flags: review?(julien.pierre.boogz)
Comment on attachment 288152 [details] [diff] [review] Use SEC_ASN1EncodeItem instead of DER_Encode I forgot to mention two things. 1. I have to initialize the 'data' member of SECItem to NULL (0) because SEC_ASN1EncodeItem asserts that dest->data is NULL. DER_Encode doesn't have that assertion. 2. I removed some out-of-date comments from lib/util.
Reopening since this bug has a patch attached that is under active consideration. Maybe the work to get rid of the old DER encoder should be in a separate bug. I seem to recall reading a message from Bob in the last few weeks, in which he observed that there was some subtle difference between the way that the old DER encoder and the newer ASN1 encoder encoded AlgorithmIDs with null parameters. One encoded them with the parameter empty and the other with an explicit DER NULL, or something like that. He explained the history of this, and said that we were keeping the DER version because of that difference, and using it in places where we want that difference to be maintained. I am unable to find that email message. We could use that knowledge now, if we had it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I would also suggest we don't reopen this bug every time we want to make a change to the util shared library. Util is now in a shared library and has been since 10/11/2007. Other changes should be in separate bugs.
Comment on attachment 288152 [details] [diff] [review] Use SEC_ASN1EncodeItem instead of DER_Encode I opened a new bug (bug 404200) for this patch.
Attachment #288152 - Attachment is obsolete: true
Attachment #288152 - Flags: superreview?(rrelyea)
Attachment #288152 - Flags: review?(julien.pierre.boogz)
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: