Closed Bug 993489 Opened 7 years ago Closed 7 years ago

ECC decode refactoring needed to build OpenJDK SunEC provider for ECC support

Categories

(NSS :: Libraries, defect, P2)

3.16
x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.16.1

People

(Reporter: elio.maldonado.batiz, Assigned: elio.maldonado.batiz)

References

Details

Attachments

(1 file, 2 obsolete files)

OpenJDK's SunEC provider currently suppports ecc via duplicated code from NSS and they need to have the SunEC provider use the system NSS instead. More details can be found on https://bugzilla.redhat.com/show_bug.cgi?id=1075702.
After Bob's analysis this is best accomplished by ecc decode refactoring that moves it from softoken to freebl.
Assignee: nobody → emaldona
Attachment #8403404 - Flags: superreview?(wtc)
Attachment #8403404 - Flags: review?(rrelyea)
Target Milestone: --- → 3.16.1
Priority: -- → P2
Duplicate of this bug: 993488
Comment on attachment 8403404 [details] [diff] [review]
Refactor ecdecode from softoken to freebl

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

::: lib/freebl/ecdecode.c
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please make sure you use the right hg command to move this file,
so that the revision history is preserved.

::: lib/freebl/loader.h
@@ +710,2 @@
>    /* Add new function pointers at the end of this struct and bump
>     * FREEBL_VERSION at the beginning of this file. */

Please bump REEBL_VERSION at the beginning of this file.

::: lib/freebl/stubs.c
@@ +522,5 @@
> +extern SECOidTag
> +SECOID_FindOIDTag_stub(const SECItem *oid)
> +{
> +    STUB_SAFE_CALL1(SECOID_FindOIDTag_Util, oid);
> +    abort();

Should we return something to avoid a compiler warning?
(In reply to Wan-Teh Chang from comment #3)
> Comment on attachment 8403404 [details] [diff] [review]
> Refactor ecdecode from softoken to freebl
> 
> Review of attachment 8403404 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/freebl/ecdecode.c
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Please make sure you use the right hg command to move this file,
> so that the revision history is preserved.
> 

I think the proper hg command is hg rename lib/softokn/ecdecode.c lib/freebl/ecdecode.c

> ::: lib/freebl/loader.h
> @@ +710,2 @@
> >    /* Add new function pointers at the end of this struct and bump
> >     * FREEBL_VERSION at the beginning of this file. */
> 
> Please bump REEBL_VERSION at the beginning of this file.
That would be from 0x0310 to 0x0311, isn't it?

> 
> ::: lib/freebl/stubs.c
> @@ +522,5 @@
> > +extern SECOidTag
> > +SECOID_FindOIDTag_stub(const SECItem *oid)
> > +{
> > +    STUB_SAFE_CALL1(SECOID_FindOIDTag_Util, oid);
> > +    abort();
> 
> Should we return something to avoid a compiler warning?

Yes, I think we should return SEC_OID_UNKNOWN.
Comment on attachment 8403404 [details] [diff] [review]
Refactor ecdecode from softoken to freebl

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

::: lib/freebl/loader.h
@@ +710,2 @@
>    /* Add new function pointers at the end of this struct and bump
>     * FREEBL_VERSION at the beginning of this file. */

FREEBL_VERSION should be changed from 0x0310 to 0x0311. (0x11 is 17 decimal.)
Address wtc's review comments. It lacks white space clean up.
Attachment #8403404 - Attachment is obsolete: true
Attachment #8403404 - Flags: superreview?(wtc)
Attachment #8403404 - Flags: review?(rrelyea)
Attachment #8403700 - Flags: review?(wtc)
Like V2 but also cleaned up white space.
Attachment #8403700 - Attachment is obsolete: true
Attachment #8403700 - Flags: review?(wtc)
Attachment #8404069 - Flags: review?(wtc)
Attachment #8404069 - Flags: superreview?(rrelyea)
Attachment #8404069 - Flags: superreview?(rrelyea) → superreview+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Previous commit had the 'hg rename' missing for some reason, i'm still new to the ways or mercurial
Added the missing part: https://hg.mozilla.org/projects/nss/rev/4a6b3e8efd9f
I had to back out the last two changesetss. They don't do what I expected.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
(In reply to Elio Maldonado from comment #4)
> (In reply to Wan-Teh Chang from comment #3)
> > Comment on attachment 8403404 [details] [diff] [review]
> > Refactor ecdecode from softoken to freebl
> > 
> > Review of attachment 8403404 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: lib/freebl/ecdecode.c
> > @@ +1,1 @@
> > > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > 
> > Please make sure you use the right hg command to move this file,
> > so that the revision history is preserved.
> > 
> 
> I think the proper hg command is hg rename lib/softokn/ecdecode.c
> lib/freebl/ecdecode.c

Thatt's not enough and had to do an explicit add and remove but the history is indeed preserved. An hg log lib/freebl/ecdecode.c shows you only the last commit but you can still see the old history in the old directory via hg lib/softoken/ecdecode.c even though the file has been moved.
Attachment #8404069 - Flags: review?(wtc)
You need to log in before you can comment on or make changes to this bug.