402285 - built-in root certs module shows no slot name

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

The command
   modutil -list   
shows this:

  2. Root Certs
        library name: C:/usr/nelsonb/.netscape/nssckbi.dll
         slots: 1 slot attached
        status: loaded

         slot:
        token: Builtin Object Token

Notice the empty/blank slot name. 
Sun would like that fixed.  
I propose "Builtin Object Slot".

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Hmm.  Looking at 
I see

 75 NSS_IMPLEMENT_DATA const NSSUTF8 *
 76 nss_builtins_SlotDescription = (NSSUTF8 *) "NSS Builtin Objects";

 88 NSS_IMPLEMENT_DATA const NSSUTF8 *
 89 nss_builtins_TokenLabel = (NSSUTF8 *) "Builtin Object Token";

So the question is: why doesn't that name appear?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

That last comment was supposed to say:
Looking at http://lxr.mozilla.org/security/source/security/nss/lib/ckfw/builtins/constants.c#75
I see ...

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: stop exporting all extern data elements from the DLL

It appears that the immediate cause of the problem was some sort of compiler
or linker error.  As originally coded, there was an unnecessary additional 
level of indirection involved.  Removing that fixes the problem.

I also noticed that the DLL exported the symbols for many of the internally 
declared private data items, items whose symbols should *NOT* be accessible 
from outside of the DLL.  This was due to use of NSS's version of the 
PR_IMPLEMENT_DATA macro. I think there must have been a misunderstanding, 
and someone thought that all "extern" symbols should be converted to use
NSS's equivalent of PR_EXTERN_DATA and PR_IMPLEMENT_DATA.  So, I just 
removed those macros from the relevant builtins code.  That reduced the 
number of exported symbols a bunch.  There are still lots more to be fixed
in ckfw, but I'll file a separate bug on that.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I wonder if, on some platforms, there is a problem with a definition like this:
const unsigned char foo[] = { "abc" };

Is it possible that quoted strings are not acceptable as initializers for 
arrays of UNSIGNED char ?

Comment 5

[Wan-Teh Chang]

Comment on attachment 287185 [details] [diff] [review]
stop exporting all extern data elements from the DLL

r=wtc.  I don't understand what the problem is, for example, why
the token label works but the slot description doesn't.  But the
patch is correct.

In builtins.h, you reordered the declarations by grouping variables
of the same type together.  This is a bad idea because the original
order matches the order in which these variables are declared in
CK_INFO, CK_SLOT_INFO, and CK_TOKEN_INFO.  Similarly it is a bad
idea to reorder the definitions of these variables in constants.c.

You can initialize a char array (NSSUTF8 is typedef'd as char) with
a string literal.  No need to put the string literal in braces.

Comment 6

[Wan-Teh Chang]

To elaborate:

NSSUTF8 is char.  CK_CHAR and CK_UTF8CHAR is unsigned char.

The arrays in question are NSSUTF8 arrays.  So they can be initialized by
string literals.  (K&R 2nd Ed. p. 219 uses the term "character array" as
opposed to "char array", so it's not clear whether an unsigned char array
is also a character array.  But a char array definitely is.)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

On trunk:
Checking in binst.c;       new revision: 1.5;  previous revision: 1.4
Checking in bslot.c;       new revision: 1.4;  previous revision: 1.3
Checking in btoken.c;      new revision: 1.4;  previous revision: 1.3
Checking in builtins.h;    new revision: 1.6;  previous revision: 1.5
Checking in certdata.c;    new revision: 1.46; previous revision: 1.45
Checking in certdata.perl; new revision: 1.12; previous revision: 1.11
Checking in constants.c;   new revision: 1.13; previous revision: 1.12

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 287185 [details] [diff] [review]
stop exporting all extern data elements from the DLL

seeking second review for branch

Comment 9

[Alexei Volkov]

Comment on attachment 287185 [details] [diff] [review]
stop exporting all extern data elements from the DLL

r=alexei

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: regenerated patch for branch

When I applied the first patch (above) to the branch and then re-ran the 
perl script that generates certdata.c from certdata.txt, and then 
re-generated the patch for the branch from the diffs, there were some
unexpected differences between the patch for the trunk and the patch for
the branch.  

These differences fall into two groups:
a) the number of built-in certs is different on the trunk than on the
branch, and 
b) the definitions of certain symbols have been reordered.  e.g.
cko_netscape_trust
cko_true
cko_certificate
The reason for this reordering is unclear.  It doesn't seem wrong or 
bad, but it is unexpected, and I want to look into it before committing
this patch.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: regenerated patch for branch, perl script sync'ed with trunk

ok, the different ordering of those symbols in certdata.c was due to 
a revision made to the perl script on the trunk but not on the branch.
the revision caused the symbols to be sorted.  I think it is desirable
for the trunk and branch to be kept in sync with respect to this perl
script and the certdata.c file it produces.  So this patch includes
the one-line change to the perl script that was also on the trunk.

Alexei, please review this patch for the branch.

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 298792 [details] [diff] [review]
regenerated patch for branch, perl script sync'ed with trunk

The purpose of this bug was to fix the slot name. You achieve this with:

+nss_builtins_SlotDescription[] = { "NSS Builtin Objects" };

which was previously an empty string.

In addition your patch contains a big amount of changes of macro uses. These changes were already in the previous patch patch revision, so we can carry forward the review of that change.

I confirm your change to the loop code in certdata.perl makes it match the trunk.

r=kengert

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

On Branch:
Checking in binst.c;       new revision: 1.3.28.2;  previous rev: 1.3.28.1
Checking in bslot.c;       new revision: 1.3.28.1;  previous rev: 1.3
Checking in btoken.c;      new revision: 1.3.28.1;  previous rev: 1.3
Checking in builtins.h;    new revision: 1.5.28.1;  previous rev: 1.5
Checking in certdata.c;    new revision: 1.36.24.8; previous rev: 1.36.24.7
Checking in certdata.perl; new revision: 1.10.28.1; previous rev: 1.10
Checking in constants.c;   new revision: 1.10.28.2; previous rev: 1.10.28.1