If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Do not export blapi.h, secrng.h, and pqgutil.h.

RESOLVED FIXED in 3.9

Status

NSS
Libraries
--
trivial
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

15 years ago
The freebl header files blapi.h and blapit.h are exported.
They should only be used inside NSS (as part of the softoken)
and should not be exported.

I'm going to move them from the EXPORTS to the PRIVATE_EXPORTS
list in freebl's manifest.mn.
(Assignee)

Comment 1

15 years ago
This tuns out to be more work than I thought because some
other public NSS headers such as secrng.h, pk11pqg.h, and
pqgutil.h include blapi.h or blapit.h.  Then I found the real
showstopper -- PSM needs the PQGParams type defined in
blapit.h.  See http://lxr.mozilla.org/seamonkey/ident?i=PQGParams.

I am afraid that I'll have to resolve this bug WONTFIX or INVALID.
Status: NEW → ASSIGNED
(Assignee)

Comment 2

15 years ago
Created attachment 124104 [details] [diff] [review]
Proposed patch

I found that Nelson pointed out in his email (which I didn't read carefully)
that we may need to make blapit.h public if other public headers use
the structure types defined in it.  This is indeed the case. 

This patch makes blapi.h private but leaves blapit.h public.  Public
headers that include blapi.h are changed to include blapit.h instead.
(Assignee)

Updated

15 years ago
Attachment #124104 - Flags: superreview?(nelsonb)
Attachment #124104 - Flags: review?(relyea)

Comment 3

15 years ago
Comment on attachment 124104 [details] [diff] [review]
Proposed patch

pretty straightforward.
r=relyea
Attachment #124104 - Flags: review?(relyea) → review+
I wonder if there are any source files in NSS that use blapi functions without
including blapi.h directly.  Perhaps they include another file that (until this 
patch) included blapi.h.  With this patch, such source files would continue to
compile without errors, but they might cause some new warnings about calls to
undeclared functions on those platforms whose compilers warn about such things.

So, my question is, do we know that this patch does not cause any new warnings
on any platforms?   If yes, then sr=nelsonb.

I'd suggest you check this patch in on the trunk, and if it doens't generate any
new warnings in the build logs on any platforms, then move the tag for this.
(Assignee)

Comment 5

15 years ago
Created attachment 124157 [details] [diff] [review]
Some files now need to include blapi.h

Good point, Nelson, and indeed you are right.  I
forgot about this.  Good that I asked for your
review.

Four files (unix_rand.c, win_rand.c, swfutl.c, and
certcgi.c) were indirectly including blapi.h via
secrng.h or pqgutil.h.	In this patch I have them
include blapi.h directly.  There may be a better fix:
moving or duplicating the RNG_ and PQG_ function
declarations they need in secrng.h and pqgutil.h.
What do you think?
(Assignee)

Comment 6

15 years ago
Continuing the previous comment.

The functions in question are:
  RNG_RandomUpdate
  RNG_GenerateGlobalRandomBytes
  PQG_ParamGen

The reason I suggested moving or duplicating their
declarations to secrng.h or pqgutil.h is that almost
all of the PQG_ functions' declarations are duplicated
in blapi.h and pqgutil.h.  So I am wondering if it
makes sense to duplicate PQG_ParamGen's declaration
in pqgutil.h.
(Assignee)

Comment 7

15 years ago
Created attachment 124299 [details] [diff] [review]
Some files now need to include blapi.h. certcgi.c should include pk11pqg.h.

Bob told me that certcgi should not include blapi.h.
So I replaced the PQG_ functions in certcgi by the
corresponding PK11_PQG_ functions (declared in pk11pqg.h).

It is fine for swutil.c, unix_rand.c, and win_rand.c to
include blapi.h.
Attachment #124157 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124299 - Flags: superreview?(nelsonb)
Attachment #124299 - Flags: review?(relyea)
Comment on attachment 124104 [details] [diff] [review]
Proposed patch

sr=nelsonb provided that both patches #124104 and #124299 are applied.
Attachment #124104 - Flags: superreview?(nelsonb) → superreview+
Comment on attachment 124299 [details] [diff] [review]
Some files now need to include blapi.h. certcgi.c should include pk11pqg.h.

sr=nelsonb provided that both patches #124104 and #124299 are applied.
Attachment #124299 - Flags: superreview?(nelsonb) → superreview+
(Assignee)

Comment 10

15 years ago
Should secrng.h be a public or private header file?  It declares
three functions: RNG_GetNoise, RNG_SystemInfoForRNG, and
RNG_FileForRNG.

Should pqgutil be a public or private header file?

Comment 11

15 years ago
RNG is definately NSS Private. The public random functions are
PK11_GenerateRandom and it's related functions.

RNG is only used by PKCS #11 modules (softoken, swfort) and low level test programs.

pqgutil.h should also be private. public applications should use pk11pqg.h. It
probably should live in freebl rather than util.

bob

(Assignee)

Comment 12

15 years ago
Created attachment 124471 [details] [diff] [review]
secrng.h and pqgutil.h should be private header files

This patch should be applied on top of the previous two patches.

1. Undo the changes made by the previous two patches to secrng.h,
pqgutil.h, swfutl.c, unix_rand.c, and win_rand.c.
2. Make secrng.h and pqgutil.h private.
3. Public header pk11pqg.h can't include pqgutil.h now that it's
private.
4. Many files don't need to include secrng.h or pqgutil.h.
(Assignee)

Comment 13

15 years ago
Bob, I found that pqgutil.{c,h} can't be moved to freebl
because pk11wrap/pk11pqg.c needs them.  Given this, should
pgqutil.h still be a private header?
(Assignee)

Comment 14

15 years ago
The PQG_ functions that pk11wrap/pk11pqg.c needs are:
PQG_DestroyParams
PQG_DestroyVerify
PQG_NewParams
PQG_GetPrimeFromParams
PQG_GetSubPrimeFromParams
PQG_GetBaseFromParams
PQG_NewVerify
PQG_GetCounterFromVerify
PQG_GetSeedFromVerify
PQG_GetHFromVerify

Comment 15

15 years ago
No, it should stay in util, then. It shouldn't be public because the functions
is defines are not exported.

bob
(Assignee)

Comment 16

15 years ago
Created attachment 124502 [details] [diff] [review]
secrng.h and pqgutil.h should be private header files, v2

The only change from the previous version is that pk11wrap/pk11pqg.c
needs to include pqgutil.h because it calls the PQG_ functions I
listed in comment 14.
Attachment #124471 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124502 - Flags: review?(nelsonb)
Comment on attachment 124502 [details] [diff] [review]
secrng.h and pqgutil.h should be private header files, v2

This patch is all fine, except for the very last change, namely to pqgutil.h.  
I'd suggest that all the PQG functions should be declared in pqgutil.h and 
blapi.h should include it, rather than the other way around.  There shouldn't 
be duplication of function declarations between the two headers.
Attachment #124502 - Flags: review?(nelsonb) → review-
(Assignee)

Comment 18

15 years ago
Created attachment 124526 [details] [diff] [review]
Combined patch

I attached a combined patch for easier review.

Summary of changes:

1. Make blapi.h, secrng.h, and pqgutil.h private.
blapit.h remains public.

2. Public header pk11pqg.h cannot include private
header pqgutil.h.  Public header cryptohi.h cannot
include private header blapi.h.

3. Many files don't need to include secrng.h.  A
couple of files don't need to include pqgutil.h.

4. certcgi.c should include pk11pqg.h instead of
pqgutil.h.

5. Remove duplicate declaration of PQG_DestroyParams
and PQG_DestroyVerify from blapi.h.  They are defined
in lib/util/pqgutil.c and therefore should be declared
in lib/util/pgqutil.h.

Turns out that PQG_DestroyParams and PQG_DestroyVerify
are the only PQG_ functions that are declared in both
blapi.h and pqgutil.h.	Other PQG_ functions are only
declared in one header file, and I found the rule: the
PQG_ functions defined in lib/util/pqgutil.c are declared
in lib/util/pqgutil.h and those defined in lib/freebl/pqg.c
are declared in lib/freebl/blapi.h.  Nelson, this is why
I didn't make the change (move all PQG_ function
declarations to pqgutil.h) you suggested.
Attachment #124104 - Attachment is obsolete: true
Attachment #124299 - Attachment is obsolete: true
Attachment #124502 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124526 - Flags: review?(nelsonb)
Comment on attachment 124526 [details] [diff] [review]
Combined patch

r=nelsonb
Attachment #124526 - Flags: review?(nelsonb) → review+
(Assignee)

Comment 20

15 years ago
Patch checked in on the tip (3.9).
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Summary: Do not export blapi.h and blapit.h → Do not export blapi.h, secrng.h, and pqgutil.h.
Target Milestone: --- → 3.9
(Assignee)

Comment 21

15 years ago
Created attachment 124605 [details] [diff] [review]
Additional patch

Also need this patch to export pqgutil.h as NSS private header
because the default makefile target "all" does not invoke the
"private_export" target.

Updated

14 years ago
Attachment #124299 - Flags: review?(rrelyea0264) → review+
You need to log in before you can comment on or make changes to this bug.