314098 - getProvider().toString() returns "Mozilla-JSS version 3.3" in JSS 4.x

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect)

Description

[Sandeep Konchady]

When printing getProvider().toString() JSS 4.x returns "Mozilla-JSS version 3.3" which is incorrect.  Here is Wan-Teh's response.

This version number is specified in org/mozilla/jss/JSSProvider.java.
It was initially set to 3.2 in JSS_3_2_RTM (the first JSS release
with a JCE provider), and only updated once to 3.3 in JSS_3_3_RTM.
This is another file that needs to be updated for each new JSS
release.

Note that the version is specified as a double, so we may not
be able to say 4.1.1.  One option is to use 4.11 to mean 4.1.1,
but if we ever release JSS 4.11 (NSS is already at 3.11, so this
is possible), we won't be able to express that.

Comment 1

[Sandeep Konchady]

Attachment: Updated JSS Provider version from 3.3 to 4.11

Updated JSS Provider version from 3.3 to 4.11

Comment 2

[Wan-Teh Chang]

Comment on attachment 201051 [details] [diff] [review]
Updated JSS Provider version from 3.3 to 4.11

With this patch applied, what does getProvider().toString()
return?

Comment 3

[Sandeep Konchady]

getProvider().toString() now return 

main: jss library loaded
Provides Signature, Message Digesting, and RNG

Using Mozilla-JSS version 4.11 provider

Comment 4

[Wan-Teh Chang]

Comment on attachment 201051 [details] [diff] [review]
Updated JSS Provider version from 3.3 to 4.11

In that case, we should set the version to 4.1
for JSS 4.1.1, that is, we should omit the "micro"
version, at least until we find out what's the
right way to express the "micro" version in the
version number.

We can still override the toString() method to print
the full version (major.minor.micro).

Comment 5

[Sandeep Konchady]

Makes sense.  While overriding toString(), do we define micro version locally in this file again or is there a way to read this information elsewhere?  The only place where this is defined completely is in org/mozilla/jss/util/jssver.h  Please advice.

Comment 6

[Wan-Teh Chang]

Let's just set it to 4.1 and worry about toString()
later.  There are many other bugs more important than
this one.  It's not easy for Java code to use jssver.h.

Please also update the comment in
mozilla/security/jss/manifest.mn.  It documents which
files contain JSS versions.  I remember you also
documented this information somewhere...

Comment 7

[Sandeep Konchady]

Attachment: Modified JSSProvider.java to report 4.1.1

Added JSS major,minor,micro version to JSSProvider.java along with overriding toString().  Also listed this as a file that needs to be changed whenever there is a change in JSS revision in the following places
lib/manifest.mn
org/mozilla/jss/CryptoManager.c
org/mozilla/jss/CryptoManager.java
org/mozilla/jss/JSSProvider.java
org/mozilla/jss/util/jssver.h

The output of getProvider().toString() now looks like this:

main: jss library loaded
Provides Signature, Message Digesting, and RNG

Using Mozilla-JSS version 4.1.1 provider

Comment 8

[Wan-Teh Chang]

Comment on attachment 201068 [details] [diff] [review]
Modified JSSProvider.java to report 4.1.1

Please change JSS_MICRO_VERSION to JSS_PATCH_VERSION
before you check in.  The use of "patch" instead of
"micro" is consistent with the JSS_VPATCH macro name
in jssver.h and the common versioning scheme documented
in http://apr.apache.org/versioning.html.

Comment 9

[Sandeep Konchady]

Checking in the code with modifications to JSSProvider.java to replace JSS_MICRO_VERSION to JSS_PATCH_VERSION.  The output of getProvider().toString() now looks like this:

main: jss library loaded
Provides Signature, Message Digesting, and RNG

Using Mozilla-JSS version 4.1.1 provider


Also the changes to JSSProvider.java from JSS_MICRO_VERSION to JSS_PATCH_VERSION  is listed below

+    private static String JSS_MAJOR_VERSION  = "4";
+    private static String JSS_MINOR_VERSION  = "1";
+    private static String JSS_PATCH_VERSION  = "1";
+    private static Double JSS_VERSION        = new Double(JSS_MAJOR_VERSION +
+                                                         "."+JSS_MINOR_VERSION);
+
     public JSSProvider() {
-        super("Mozilla-JSS", 3.3,
+        super("Mozilla-JSS", JSS_VERSION,
                 "Provides Signature, Message Digesting, and RNG");

         /////////////////////////////////////////////////////////////
@@ -205,4 +223,11 @@
             "org.mozilla.jss.provider.javax.crypto.JSSMacSpi$HmacSHA1");
         put("Alg.Alias.Mac.Hmac-SHA1", "HmacSHA1");
     }
+
+    public String toString() {
+        return "Mozilla-JSS version " +
+               JSS_MAJOR_VERSION + "." +
+               JSS_MINOR_VERSION + "." +
+               JSS_PATCH_VERSION;
+    }


Checkin Log:
------------
Checking in lib/manifest.mn;
/cvsroot/mozilla/security/jss/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.7; previous revision: 1.6
done
Checking in org/mozilla/jss/CryptoManager.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/CryptoManager.c,v  <--  CryptoManager.c
new revision: 1.14; previous revision: 1.13
done
Checking in org/mozilla/jss/CryptoManager.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/CryptoManager.java,v  <--  CryptoManager.java
new revision: 1.37; previous revision: 1.36
done
Checking in org/mozilla/jss/JSSProvider.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/JSSProvider.java,v  <--  JSSProvider.java
new revision: 1.15; previous revision: 1.14
done
Checking in org/mozilla/jss/util/jssver.h;
/cvsroot/mozilla/security/jss/org/mozilla/jss/util/jssver.h,v  <--  jssver.h
new revision: 1.24; previous revision: 1.23
done

Comment 10

[Sandeep Konchady]

Attachment: Changed Double to Double.doubleValue() in constructing java.Security.Provider()

JDK 5.0 does not complain when I use Double, but JDK 1.4.x does.  This needs to be double (native). Changed Double to Double.doubleValue() in constructing java.Security.Provider()

Comment 11

[Wan-Teh Chang]

Comment on attachment 201143 [details] [diff] [review]
Changed Double to Double.doubleValue() in constructing java.Security.Provider()

You can also just make JSS_VERSION a double.

Another thing we should fix is that toString()
should not append "." + JSS_PATCH_VERSION if
JSS_PATCH_VERSION is "0".  We say JSS 4.1, not
4.1.0.

It seems that JSS_{MAJOR,MINOR,PATCH}_VERSION
can be declared as int rather than String.

Finally, we can express our version in double as
follows:

JSS 4.0: 4.0
JSS 4.1: 4.01
JSS 4.1.1: 4.0101

We use the first two digits after the decimal
point to represent the minor version, and the
next two digits to represent the patch version.
This version encoding allows us to have more
than 10 (up to 99) minor versions or patch versions.
We should find out how the SUN provider encodes
its version.

Comment 12

[Sandeep Konchady]

Attachment: Changed JSS_VERSION to report 4.1 or 4.1.1 based on versions.

As per Wan-Teh's recommendation changed JSS_VERSION to report 4.1 or 4.1.1 based on versions. Modified toString() as well as constructor.  Added a convenience method round(double, int) to round off.

Comment 13

[Wan-Teh Chang]

Comment on attachment 201143 [details] [diff] [review]
Changed Double to Double.doubleValue() in constructing java.Security.Provider()

Please check in this patch today to fix the JSS
build failure with JDK 1.4.  I may not have time
to review your new patch today.

Comment 14

[Sandeep Konchady]

Modified JSSProvider.java to use Double.doubleValue() instead of Double.  This broke the build in JDK 1.4.

Checking in JSSProvider.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/JSSProvider.java,v  <--  JSSProvider.java
new revision: 1.16; previous revision: 1.15
done

Comment 15

[Wan-Teh Chang]

Comment on attachment 201175 [details] [diff] [review]
Changed JSS_VERSION to report 4.1 or 4.1.1 based on versions.

I don't understand what the round() method does.

It seems that you will print the version as "4.01.1"
instead of "4.1.1".

>+        String mozillaProviderVersion = new Double(
>+                                        round(JSS_VERSION,2)).toString();

Does this work?

    String mozillaProviderVersion = JSS_MAJOR_VERSION
            + "." + JSS_MINOR_VERSION;

Comment 16

[Sandeep Konchady]

Currently private static double JSS_VERSION = round((double)(JSS_MAJOR_VERSION +
(double)JSS_MINOR_VERSION/10),2);  What round(double,2) does is that it rounds off the double to two decimal points.  Hence JSS_VERSION = 4.1 and not 4.01.  That is why I am having JSS_MINOR_VERSION divided by 10 (1/10 = 0.1).

The output now looks like this: "Using Mozilla-JSS version 4.1.1 provider"

If I set JSS_PATCH_VERSION to 0, then the output looks like this: 
"Using Mozilla-JSS version 4.1 provider"

Comment 17

[Sandeep Konchady]

When testing JSS Provider Version after JSS was updated to 4.2 I found a bug in the implementation.  The revision was being reported as 4.21 instead of 4.2.  Hence I am reopening this bug.

Comment 18

[Sandeep Konchady]

Attachment: rounding of double(4) + double(1)/10 to two decimal places gives 4.21

rounding of double(4) + double(1)/10 to two decimal places gives 4.21 instead of 4.2 in JSSProvider.java.  Corrected this and we now get 4.2 when the patch version is 0 and 4.2.1 if patch version is > 0.

main: jss library loaded
Provides Signature, Message Digesting, and RNG

Using Mozilla-JSS version 4.2 provider

Comment 19

[Sandeep Konchady]

Checking in JSSProvider.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/JSSProvider.java,v  <--  JSSProvider.java
new revision: 1.18; previous revision: 1.17
done

Comment 20

[Sandeep Konchady]

Adding Nelson and Wan-Teh's comments

Nelson B Bolyard wrote:

> [e-mail] wrote:
>  
>
>> Diffs: http://bonsai.mozilla.org/cvsview2.cgi?command=DIRECTORY&subdir=mozilla/security/jss/org/mozilla/jss&files=JSSProvider.java&root=/cvsroot
>>
>> Update of /cvsroot/mozilla/security/jss/org/mozilla/jss
>> In directory megalon:/tmp/cvs-serv25987
>>
>> Modified Files:
>>     JSSProvider.java Log Message:
>> Bugzilla bug: 314098
>> Rounding of double(4) + double(1)/10 to two decimal places gives 4.21.
>> Fixed this issue.  sr=glen.beasley   
>
>
> I looked at this patch, and concluded that there is a typo in that
> description, and that the actual problem was round(4+double(2)/10)
> (2 not 1).
>
> I have a number of concerns about this solution.
>
> 1) You found a problem with the value 4.2, and you found a workaround
> for that value.
> Does that solution also work for 4.3, 4.4, ... 4.9, 4.10 ...?
> Is the code going to need to be changed again for a later version number?
>
> 2) Suppose that someday you produce a version 4.10.
> when treated as a floating point number,
> how will that be different from 4.1 ?
> Should the version numbers be expressed in hundredths? e.g. 4.01, 4.02 ... ?
>
> 3) why doesn't round(4+double(2)/10) produce the expected result?
> Is this a bug in round() ?  Should it be reported to the Java folks?
> Does Java have different rules for converting integers to floats/doubles
> than c does?
> Is the problem due to double rounding?  (e.g. rounding done by round()
> and more rounding done by the function that prints it out) ?
>
> /Nelson Nelson B Bolyard wrote:

Wan-Teh Chang wrote:

If we want to have the flexibility of releasing
JSS 4.10 or 4.1.10, we need to be able to express
a two digit version component in the double version.
A good solution is to use the first two digits after
the decimal point for the minor version, and the next
two digits for the patch level.  So:

Human readable version       Double version
-------------------------------------------
4.1                          4.01
4.1.1                        4.0101
4.1.10                       4.0110
4.2                          4.02
4.10                         4.10

I believe the double version is only used for fast
comparison of versions.  For display, the toString
method should be used, and we should override our
toString method to print the human readable version.

Wan-Teh

Comment 21

[Sandeep Konchady]

Attachment: Removed the round method and converted JSS_VERSION to reflect 4.0200

Removed the round method and converted double JSS_VERSION to reflect JSS_MAJOR_VERSION.(JSS_MINOR_VERSION+JSS_PATCH_VERSION) = 4.0200.  However human readable version would read 4.2.  These are some sample output:

Case 1:
JSS_MAJOR_VERSION=4
JSS_MINOR_VERSION=2
JSS_PATCH_VERSION=1
main: jss library loaded
JSS_VERSION used in constructor is 4.0201
Provides Signature, Message Digesting, and RNG

Using Mozilla-JSS version 4.2.1 provider

Case 2:
JSS_MAJOR_VERSION=4
JSS_MINOR_VERSION=2
JSS_PATCH_VERSION=0
main: jss library loaded
JSS_VERSION used in constructor is 4.02
Provides Signature, Message Digesting, and RNG

Using Mozilla-JSS version 4.2 provider

Case 3:
JSS_MAJOR_VERSION=4
JSS_MINOR_VERSION=10
JSS_PATCH_VERSION=0
main: jss library loaded
JSS_VERSION used in constructor is 4.1
Provides Signature, Message Digesting, and RNG

Using Mozilla-JSS version 4.10 provider

Case 4:
JSS_MAJOR_VERSION=4
JSS_MINOR_VERSION=10
JSS_PATCH_VERSION=1
main: jss library loaded
JSS_VERSION used in constructor is 4.1001
Provides Signature, Message Digesting, and RNG

Using Mozilla-JSS version 4.10.1 provider

Comment 22

[Wan-Teh Chang]

Comment on attachment 203441 [details] [diff] [review]
Removed the round method and converted JSS_VERSION to reflect 4.0200

>+    private static String JSSVERSION      = new Float(JSS_MAJOR_VERSION +
>+                                            (float)JSS_MINOR_VERSION/100 +
>+                                            (float)JSS_PATCH_VERSION/10000).toString();
>+    private static double JSS_VERSION     = new Double(JSSVERSION).doubleValue();

You convert a float to a Float to a String to a Double to a double,
and save the intermediate String value in the private static member
JSSVERSION, which is used only to initialize JSS_VERSION.

The above can be simply

      private static double JSS_VERSION     = JSS_MAJOR_VERSION +
                                              JSS_MINOR_VERSION/100.0 +
                                              JSS_PATCH_VERSION/10000.0;

Comment 23

[Wan-Teh Chang]

Comment on attachment 203441 [details] [diff] [review]
Removed the round method and converted JSS_VERSION to reflect 4.0200

Sandeep explains why he needs to construct a string from
a float and convert it back to a double: floating point
cannot represent certain values like .02 and .0001 exactly,
so if you do 4 + .02 + .0001 as I suggested, you end up with
4.020099999999999.

I believe a small change to avoid adding the two inexactly
represented floating point numbers will be able to avoid this
issue:

      private static double JSS_VERSION     = JSS_MAJOR_VERSION +
                                              (JSS_MINOR_VERSION * 100 +
                                              JSS_PATCH_VERSION)/10000.0;

Sandeep, could you try this?

Comment 24

[Sandeep Konchady]

Attachment: Modified the computation of JSS_VERSION based on Wan-Teh's suggestion

Modified the computation of JSS_VERSION based on Wan-Teh's suggestion.  This works perfect and the double values match the format that we are interested in (EG: 4.0200 for 4.2 or 4.1001 for 4.10.1)

Comment 25

[Wan-Teh Chang]

Comment on attachment 203558 [details] [diff] [review]
Modified the computation of JSS_VERSION based on Wan-Teh's suggestion

r=wtc.

Comment 26

[Sandeep Konchady]

Checking in JSSProvider.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/JSSProvider.java,v  <--  JSSProvider.java
new revision: 1.19; previous revision: 1.18
done

Comment 27

[Wan-Teh Chang]

Sandeep, this change needs to be carried back to
JSS_4_1_BRANCH, otherwise JSS 4.1.1's double version
(currently 4.1) will be greater than JSS 4.2's double
version (4.02).

Comment 28

[Sandeep Konchady]

Attachment: Changes made to tip being ported in JSS_4_1_BRANCH

Changes made to tip being ported in JSS_4_1_BRANCH.  JSS Provider Version set to 4.1.1

Comment 29

[Wan-Teh Chang]

Comment on attachment 203587 [details] [diff] [review]
Changes made to tip being ported in JSS_4_1_BRANCH

r=wtc.

Comment 30

[Sandeep Konchady]

Backported the changes from tip to JSS_4_1_BRANCH.

Checking in JSSProvider.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/JSSProvider.java,v  <--  JSSProvider.java
new revision: 1.16.2.1; previous revision: 1.16
done