286642 - util should be in a shared library

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

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.

Comment 1

[Wan-Teh Chang]

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.

Comment 2

[Julien Pierre]

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.

Comment 3

[Robert Relyea]

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

Comment 4

[Robert Relyea]

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.

Comment 5

[Julien Pierre]

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 .

Comment 6

[Robert Relyea]

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

Comment 7

[Julien Pierre]

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 .

Comment 8

[Robert Relyea]

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

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[Robert Relyea]

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

Comment 12

[Julien Pierre]

Do we want to fix this for 3.11 ?

Comment 13

[Julien Pierre]

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.

Comment 14

[Julien Pierre]

Setting target to 3.12 per our meeting today.
Who should we reassign this to now that Saul has left ?

Comment 15

[Robert Relyea]

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

Comment 16

[Julien Pierre]

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.

Comment 17

[Robert Relyea]

Attachment: Build nssutil as a shared library.

Testing and comments on other platforms welcome.

bob

Comment 18

[Julien Pierre]

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 ?

Comment 19

[Robert Relyea]

Attachment: Missing .def and .rc files

These go with the previous patch. They are necessary to build libutil.so

Comment 20

[Robert Relyea]

from the mozilla-nssdev mailing list. Relevant to this bug posted by wtc:

On 6/8/07, Robert Relyea <[e-mail]> 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 

Comment 21

[Robert Relyea]

In our .def files the line would be:

PORT_Alloc=nssutil3.PORT_Alloc,Private ;-

So it doesn't get picked up by Unix platforms

Comment 22

[Julien Pierre]

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

Comment 23

[Julien Pierre]

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.

Comment 24

[Julien Pierre]

Same build issue with VC8 and its linker.

Comment 25

[Julien Pierre]

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.

Comment 26

[Julien Pierre]

I am working on fixing the patch for Windows. Once I fix it, I will try to check for binary compatibility.

Comment 27

[Julien Pierre]

Attachment: updated patch

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.

Comment 28

[Julien Pierre]

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.

Comment 29

[Julien Pierre]

Attachment: Fix build problems

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.

Comment 30

[Robert Relyea]

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

Comment 31

[Robert Relyea]

Attachment: NSS .def file with forwarding...

Comment 32

[Robert Relyea]

Attachment: nssutil.def updated

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 33

[Julien Pierre]

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 .

Comment 34

[Julien Pierre]

Attachment: Solaris build log trying to building libnss3.so

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 .

Comment 35

[Julien Pierre]

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.

Comment 36

[Robert Relyea]

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

Comment 37

[Julien Pierre]

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.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

sounds like the Solaris linker command may be missing a certain command line
option, (or may have a wrong option).  

Comment 39

[Robert Relyea]

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.

Comment 40

[Julien Pierre]

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.

Comment 41

[Julien Pierre]

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   |

Comment 42

[Julien Pierre]

AIX seems to be fine with the patch too !

So the ones that have problems are Solaris and Windows.

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

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>

Comment 44

[Julien Pierre]

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.

Comment 45

[Julien Pierre]

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.

Comment 46

[Robert Relyea]

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

Comment 47

[Julien Pierre]

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.

Comment 48

[Wan-Teh Chang]

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.)

Comment 49

[Julien Pierre]

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

Comment 50

[Julien Pierre]

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 .

Comment 51

[Julien Pierre]

Attachment: Updated patch

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.

Comment 52

[Wan-Teh Chang]

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;

Comment 53

[Julien Pierre]

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.

Comment 54

[Julien Pierre]

Attachment: Working patch

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.

Comment 55

[Wan-Teh Chang]

Comment on attachment 277846 [details] [diff] [review]
Working patch

"Util" would be a better prefix than "Moved".

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

I requested that the "Moved" word be a suffix, rather than a prefix.  

Comment 57

[Julien Pierre]

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.

Comment 58

[Robert Relyea]

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

Comment 59

[Julien Pierre]

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.

Comment 60

[Robert Relyea]

OK, I'll retest my linux builds. I thought I was working.

(RHEL5 & RHEL4).

bob

Comment 61

[Julien Pierre]

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.

Comment 62

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 63

[Julien Pierre]

Attachment: completed patch

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

Comment 64

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 65

[Julien Pierre]

Attachment: Implement feedback from Nelson

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]

Comment 66

[Wan-Teh Chang]

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".

Comment 67

[Julien Pierre]

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.

Comment 68

[Julien Pierre]

Attachment: Incorporate Wan-Teh's feedback. Also fix Windows and AIX issues

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).

Comment 69

[Julien Pierre]

- 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 70

[Julien Pierre]

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.

Comment 71

[Julien Pierre]

Comment on attachment 283309 [details] [diff] [review]
Implement feedback from Nelson

Deleting review request on older patch.

Comment 72

[Julien Pierre]

Attachment: Fix upgrade db crash problem, Solaris AMD64 build

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.

Comment 73

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 74

[Nelson Bolyard (seldom reads bugmail)]

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).

Comment 75

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 76

[Julien Pierre]

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.

Comment 77

[Hermann Schwab]

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

Comment 78

[Julien Pierre]

Hermann,

Actually in this case, I was referring to attachment 283650 [details] [diff] [review], not to bug 283650 .

Comment 79

[Julien Pierre]

Attachment: fake patch to help Nelson with review

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.

Comment 80

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 81

[Julien Pierre]

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.

Comment 82

[Julien Pierre]

Attachment: diff of attachment 283309 (implement feedback from Nelson) and attachment 284230 (fix upgrade DB crash problem)

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] .

Comment 83

[Nelson Bolyard (seldom reads bugmail)]

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 84

[Nelson Bolyard (seldom reads bugmail)]

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),

Comment 85

[Kai Engert (:KaiE:)]

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.

Comment 86

[Kai Engert (:KaiE:)]

Attachment: Test 1

Normalized version of attachment 283650 [details] [diff] [review]

Comment 87

[Kai Engert (:KaiE:)]

Attachment: Test 2

Normalized version of attachment 284230 [details] [diff] [review].

Comment 88

[Julien Pierre]

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. 

Comment 89

[Kai Engert (:KaiE:)]

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.

Comment 90

[Julien Pierre]

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.

Comment 91

[Nelson Bolyard (seldom reads bugmail)]

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)

Comment 92

[Julien Pierre]

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.

Comment 93

[Julien Pierre]

Attachment: Fix broken JSS build by linking to new nssutil3 shared lib

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

Comment 94

[Nelson Bolyard (seldom reads bugmail)]

Julien, looks like this checkin may have caused bug 399589. 
Please investigate that.

Comment 95

[Wan-Teh Chang]

(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.

Comment 96

[Christophe Ravel]

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

Comment 97

[Julien Pierre]

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.

Comment 98

[Julien Pierre]

I have opened bug 400094 for the JSS build issue. This bug is done for the NSS side.

Comment 99

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 100

[Julien Pierre]

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.

Comment 101

[Nelson Bolyard (seldom reads bugmail)]

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. :-/

Comment 102

[Julien Pierre]

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.

Comment 103

[Nelson Bolyard (seldom reads bugmail)]

The warning is:
secdig.c(69) : warning C4133: 'return' : incompatible types - from 
'DERTemplate [4]' to 'const SEC_ASN1Template *'

Comment 104

[Julien Pierre]

Attachment: Add typing to the macros

Comment 105

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 106

[Julien Pierre]

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.

Comment 107

[Nelson Bolyard (seldom reads bugmail)]

reopening, so that the review request will show up on my radar.
I don't see review requests for resolved bugs.

Comment 108

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative macro typing patch

Julien, I'd like to propose this alternative macro typing patch.
Please review.

Comment 109

[Wan-Teh Chang]

Comment on attachment 287191 [details] [diff] [review]
alternative macro typing patch

I like Nelson's patch better.  Let's do it.

Comment 110

[Wan-Teh Chang]

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 111

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 286250 [details] [diff] [review]
Add typing to the macros

I concur with Wan-Teh's comment 110.

Comment 112

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 287191 [details] [diff] [review]
alternative macro typing patch

obsoleting this patch, because I concur with Wan-Teh's comment 110.

Comment 113

[Robert Relyea]

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 114

[Julien Pierre]

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.

Comment 115

[Julien Pierre]

Attachment: Don't export SGNDigestInfoTemplate from libnssutil3. Copy it to cryptohi and softoken where used. (checked in)

Comment 116

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 117

[Julien Pierre]

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

Comment 118

[Wan-Teh Chang]

Attachment: Use SEC_ASN1EncodeItem instead of DER_Encode

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?

Comment 119

[Wan-Teh Chang]

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.

Comment 120

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 121

[Julien Pierre]

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 122

[Wan-Teh Chang]

Comment on attachment 288152 [details] [diff] [review]
Use SEC_ASN1EncodeItem instead of DER_Encode

I opened a new bug (bug 404200) for this patch.