863871 - Remove CVS keywords from NSS source files

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Since we switched to Mercurial, it doesn't make sense to have CVS keywords in the source code any more. Wan-Teh mentioned that they should be removed on the nss-dev list.

I removed the "-v" option to ssltap as part of doing this, since it output the CVS ID.

I found one issue with this work that I did not address: certdata.perl will embed the CVS ID information into the generated PKCS#11 module, and that data is exposed at runtime by the PKCS#11 module:


  if( /(^CVS_ID\s+)(.*)/ ) {
    $cvsid = $2 . "\"; $cvs_id\"";
    my $scratch = $cvsid;
    $size = 1 + $scratch =~ s/[^"\n]//g;
    @{$objects[0][0]} = ( "CKA_CLASS", "&cko_data", "sizeof(CK_OBJECT_CLASS)" );
    @{$objects[0][1]} = ( "CKA_TOKEN", "&ck_true", "sizeof(CK_BBOOL)" );
    @{$objects[0][2]} = ( "CKA_PRIVATE", "&ck_false", "sizeof(CK_BBOOL)" );
    @{$objects[0][3]} = ( "CKA_MODIFIABLE", "&ck_false", "sizeof(CK_BBOOL)" );
    @{$objects[0][4]} = ( "CKA_LABEL", "\"CVS ID\"", "7" );
    @{$objects[0][5]} = ( "CKA_APPLICATION", "\"NSS\"", "4" );
    @{$objects[0][6]} = ( "CKA_VALUE", $cvsid, "$size" );
    $objsize[0] = 7;
    next;
  }

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Remove CVS keywords (trivial cases)

Elio, I think you are the best reviewer because packagers like you are most likely to be affected by these changes, though I don't know of any negative effects.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Remove ssltap's -v (version) option since it is based on CVS keywords

Comment 3

[Wan-Teh Chang]

Comment on attachment 739803 [details] [diff] [review]
Part 2: Remove ssltap's -v (version) option since it is based on CVS keywords

r=wtc.

If no other NSS tool has the -v (version) option, it's fine to remove
it.

Comment 4

[Wan-Teh Chang]

Attachment: Part 1: Remove CVS keywords (trivial cases), v2, by Brian Smith

Brian:

I reviewed your Part 1 patch. Some files in the following directories
need to be manually adjusted to avoid leaving behind empty comment
lines or even empty comment blocks:
lib/certdb
lib/certhigh
lib/freebl
lib/freebl/mpi
lib/pkcs7
lib/smime
lib/softoken/legacydb
lib/util

I reverted the changes to third-party code in lib/sqlite and lib/zlib.
I believe the CVS keywords in those files come from upstream.

The CVS keyword in lib/jar/jzconf.h may have also come from zlib
upstream, but since we have forked that file, I think it's fine to
remove the CVS keyword from that file.

https://hg.mozilla.org/projects/nss/rev/9483ef9455fe

Comment 5

[Wan-Teh Chang]

Comment on attachment 739803 [details] [diff] [review]
Part 2: Remove ssltap's -v (version) option since it is based on CVS keywords

https://hg.mozilla.org/projects/nss/rev/cd2ba3fec0ce

Brian: if you don't have any other patches, please mark this bug fixed.
Thanks.

Comment 6

[Elio Maldonado]

Comment on attachment 739803 [details] [diff] [review]
Part 2: Remove ssltap's -v (version) option since it is based on CVS keywords

r+, i didn't find any dependencies that the tools would have on this.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Remove CVS ID from generated code

This removes the generation of the CVS_ID component of the NSS builtin token.

Unfortunately, because the CVS_ID component was at the start of the array, the diff between the old generated certdata.c and the new generated certdata.c is very noisy, as every variable has its index in its name, and so all variables end up getting renamed.

Note I think there was a bug in the original code, where if CVS_ID wasn't present in the input, then things would fail to work.

Comment 8

[Wan-Teh Chang]

Comment on attachment 745465 [details] [diff] [review]
Part 3: Remove CVS ID from generated code

Review of attachment 745465 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for writing the patch.

There is an off-by-one error in certdata.perl. You can see
that if you check the last array element in the generated
certdata.c file.

::: lib/ckfw/builtins/certdata.perl
@@ +6,4 @@
>  use strict;
>  
>  my %constants;
> +my $count = -1;

Why do you initialize $count to -1?

I guess this change is required for $count to start with 0
instead of 1?

@@ +92,5 @@
>  doprint();
>  
>  sub dudump {
>  my $i;
> +for( $i = 0; $i < $count; $i++ ) {

Why do you change $i <= $count to $i < $count?

I think this caused the off-by-one error.

In the generated certdata.c, the last array element is now
nss_builtins_types_373.  nss_builtins_types_373 is the certificate
data of "CA Disig Root R2".  This means certdata.c is missing the
trust settings of "CA Disig Root R2".

@@ +176,5 @@
>  
>  print "};\n";
>  
>  print "const PRUint32\n";
> +print "nss_builtins_nObjects = $count;\n";

This should be $count+1, if the for loops test the condition
$i <= $count.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch that minimizes the diff in the generated output by keeping the same (but misleading) variable names.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Diff of generated certdata.c, before and after the patch in attachment 746145 is applied

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3 [v3]: Remove CVS ID from generated code

Thanks for catching my error.

The use of $count in this program is confusing. At the beginning of the program, it holds the number of objects (0). But, most of the time, it holds the index of the current item, which is one less than the number of objects. That is, most of the time, $count is not really the count of the number of objects. This is what caused me to create the off-by-one error.

I have rewritten the code to store the current index is stored in one variable while the count of items is stored in a separate variable. This way, $count now always refers to the number of items in the arrays. This is much more clear.

attachment 746145 [details] [diff] [review] is a variant of the patch where the generated variable names are kept the same as they were before my patch. This allows you to generate the diff in attachment 746147 [details] [diff] [review], showing that the changes I made are correctly. The bad aspect of this patch is that nss_builtins_data[i] refers to nss_builtins_types_(i+1) and nss_builtins_items_(i+1).

The actual patch to review is attachment 746147 [details] [diff] [review]. It is the same as attachment 746145 [details] [diff] [review] except that it does not do extra work to maintain the same variable names that were generated in previous versions. This is clearer because now nss_builtins_data[i] refers to nss_builtins_types_i and nss_builtins_items_i+1 like before. But, the diff of this version of the patch is basically unreadable.

Comment 12

[Wan-Teh Chang]

Comment on attachment 746155 [details] [diff] [review]
Part 3 [v3]: Remove CVS ID from generated code

Review of attachment 746155 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ckfw/builtins/certdata.perl
@@ +91,3 @@
>  
>   # print "$fields[0] | $fields[1] | $size | $fields[2]\n";
>  }

I have no problem with for loops that check $i <= $count.

If you prefer to check $i < $count, we can accomplish that easily
by incrementing $count immediately after this while loop.

Right now we increment $count whenever we see the CKA_CLASS line.
So my proposed change simulates a fictitious CKA_CLASS line beyond
the end of the certdata.txt file.

Using the same variable as both an index and a count is quite
common. With a zero-based index, for the count to be correct,
the variable needs to be incremented *after* processing each item.
certdata.txt only gives us a simple way to detect the beginning
of each item, so we have to use this workaround.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3 [v4]: Remove CVS ID from generated code

Wan-teh, I do not think we should spend too much more time on this. I updated the patch to keep it simple: it now clearly just removes everything related to the 0th object, which was the CVS ID. $count always contains the correct count and no extra counters are used.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Diff of generated certdata.c before & after Part 3 [v4] [NOT FOR CHECKIN]

Comment 15

[Wan-Teh Chang]

Comment on attachment 750617 [details] [diff] [review]
Part 3 [v4]: Remove CVS ID from generated code

Review of attachment 750617 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 750617 [details] [diff] [review]
Part 3 [v4]: Remove CVS ID from generated code

https://hg.mozilla.org/projects/nss/rev/c63c14c95855

Thanks for the review, Wan-Teh (and sorry about the typo in my previous comment.)