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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

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.

Comment 1

[Wan-Teh Chang]

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.

Comment 2

[Wan-Teh Chang]

Attachment: 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.

Comment 3

[Robert Relyea]

Comment on attachment 124104 [details] [diff] [review]
Proposed patch

pretty straightforward.
r=relyea

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 5

[Wan-Teh Chang]

Attachment: 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?

Comment 6

[Wan-Teh Chang]

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.

Comment 7

[Wan-Teh Chang]

Attachment: 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.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 124104 [details] [diff] [review]
Proposed patch

sr=nelsonb provided that both patches #124104 and #124299 are applied.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Wan-Teh Chang]

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

[Robert Relyea]

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

Comment 12

[Wan-Teh Chang]

Attachment: 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.

Comment 13

[Wan-Teh Chang]

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?

Comment 14

[Wan-Teh Chang]

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

[Robert Relyea]

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

bob

Comment 16

[Wan-Teh Chang]

Attachment: 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.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 18

[Wan-Teh Chang]

Attachment: 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.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 124526 [details] [diff] [review]
Combined patch

r=nelsonb

Comment 20

[Wan-Teh Chang]

Patch checked in on the tip (3.9).

Comment 21

[Wan-Teh Chang]

Attachment: 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.