647902 - Add general purpose allocation code for CERT_PKIXVerifyCert

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

I propose to add code to NSS that simplifies the use of the CERT_PKIXVerifyCert API.

One tricky part is to deal with the nested input parameter.
I'm adding three functions that handle the allocation and cleanup.

Now the question is,
do you agree that this code is general purpose?

If yes, I'd like to get id added to NSS 3.12.10

If you don't like this code, I can add it locally in PSM.


However, even if you don't like the code in NSS, I'd still appreciate a review for correctness.


Wan-Teh, I know you don't like this API, but that's we have got, and there isn't an alternative. Short-term it won't help us to request another API.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 3

[Wan-Teh Chang]

We don't need these allocation functions in NSS.  NSS command-line
tools, PSM, and Linux Chromium have no problem constructing the input
parameters on the stack.

These functions can be added to PSM as convenience functions though.

Comment 4

[Kai Engert (:KaiE:)]

(In reply to comment #3)
> We don't need these allocation functions in NSS.

If you're saying "we", I conclude you mean Google.
Google might not need it, but other clients of NSS might be very happy about having these convenient functions available. Why require every client of NSS to reinvent the wheel?

Please clarify if you actually veto to getting these functions added to NSS.


> PSM has no problem constructing the input parameters on the stack.

Unfortunately PSM can not use the stack, because PSM must reduce the amount of times these objects get allocated. The reason is that querying the preferences requires locks and proxying to the right thread, and this is too expensive to do it each time we have to verify a certificate.

Comment 5

[Wan-Teh Chang]

I always write as a member of the NSS team in NSS bug reports.
What I meant was that NSS doesn't need these allocation functions.

The allocation functions you proposed are necessitated by your
patch in bug.  I believe that by defining the
nsCERTValInParamWrapper class differently, you can avoid
manual dynamic allocation of CERTRevocationFlags and related
structures.

For example, the "CERTRevocationFlags *mRev" member of
your nsCERTValInParamWrapper class can be defined as
"CERTRevocationFlags mRev".  That'll get rid of the
allocation of CERTRevocationFlags.  You can get rid
of the allocation of other revocation-related structures
similarly, by defining them as members of the
nsCERTValInParamWrapper class.

Comment 6

[Kai Engert (:KaiE:)]

I suggest to hear a third opinion from Bob:
Is the code in this bug helpful or unnecessary?

Note to Bob: Comment 5 is _not_ about this bug, but about code in bug 479393.


Wan-Teh: Yes, what you propose is an alternative implementation strategy. Unless we found that my code is broken or has disadvantages, it would be great if I wouldn't need to repeat my coding and testing work by coding an alternative implementation.

Comment 7

[Robert Relyea]

Right NSS does not need these functions. The question is, are they useful for other applications to use. If each application has to replicate these allocation functions themselves.

I also don't think we should dictate that these arrays must be created on the stack. While I think a stack way of setting these values is fine for some uses but I also envision passing these arrays around and reusing them for multiple connection types (why rebuild a certificate revocation policy over and over).

That being said, there may very well be problems with the existing patch, and without having looked at either patch thoroughly yet, wtc's comments on the Revocation structure seems sound (at least at first blush).

bob

Comment 8

[Wan-Teh Chang]

The coding style depends on the programmer's past
experience.

The CERTValInParam array is similar to the
"CK_ATTRIBUTE_PTR  pTemplate" argument in PKCS #11.
I have read a lot of NSS code that sets up a
pTemplate argument in PKCS #11, so I am comfortable
creating a CERTValInParam array without using
malloc/PORT_Alloc to allocate every object in that
array.

I suspect that Kai is not as familiar with setting
up this kind of input argument, so the solution
he came up with looks complicated to me.

The changes I suggested in comment 5 do not require
allocating the CERTValInParam array on the stack.

Comment 9

[Robert Relyea]

OK, looking at the allocation routines, I see the following issues:

1) As generic Allocation routines they suffer from 2 problems

a) Naming... The 'Inner' keyword in the Destroy functions don't make sense in the generic context. I think they probably make sense in Kai's code specifically, but here they should be renamed.

b) Freeing the const pointer entries in the arrays is a bit problematic. This is the same issue we had when trying to implement 'SetDefault'. I think the same rule applies here... The owner should keep track of those pointers that it 'owns' and frees them himself.

I don't have a problem with allocating the revocation structure, and having an NSS helper to do so, but I don't think we should just blindly free it in a generic InParam Free structure. It is also permissible that the revocation structure was a const static structure that was statically allocated by NSS and returned to the application as a result of a 'Give me Revocation Policy #1'. Likewise the Policy oid could be a static const string.

---------------------------------

That being said I see two ways to go. Kai could fold these functions back into PSM as PSM's own helper functions, or he could make the above changes to keep the functions as NSS patches.

bob

Comment 10

[Robert Relyea]

> The CERTValInParam array is similar to the
> "CK_ATTRIBUTE_PTR  pTemplate" argument in PKCS #11.

Ah yes a paradigm I'm quite familiar with. Internally NSS passes Templates in each of the following ways:

1) Handle contruction on the stack, with values being 'const' values.
2) Allocating Arrays , and then allocating the data for those arrays.
3) Stack allocated Arrays, but memory allocation for the data of the arrays

There may even be instances of heap allocated arrays and static/const data values.

One difference between PKCS #11 Templates and CERTValInParams is the data pointed to in the CERTVALInParams can be structures themselves. In PKCS #11, Templates represent single structures themselves.

bob

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Patch v3

I've removed function CERT_DefaultDestroyInnerCERTValInParam.
I don't need it anymore.
This avoids the "const cast" story.

I've removed function CERT_AllocCERTValInParam.
It's just one line.

I want to keep the functions for the revocation flags in NSS.

I've renamed the second "inner" function to
CERT_DestroyCERTRevocationFlags.

Because the alloc function does allocate both outer and inner data,
I've changed the destroy function to delete both.

Comment 12

[Robert Relyea]

Comment on attachment 524581 [details] [diff] [review]
Patch v3

r+ though I would like one style change.

We have a PORT_New and PORT_NewArray Macro which we try to use in all new code, so things like

PORT_Alloc(sizeof(xyz)) becomes PORT_New(xyz) (where xyz is a type)

or

PORT_Alloc(count*sizeof*xyz) becomes PORT_NewArray(xyz,count)

They also explicitly cast the point to xyz * or xyz** as appropriate.

bob

Comment 13

[Kai Engert (:KaiE:)]

Attachment: tweaked patch v3

= patch v3 + the change Bob requested

Comment 14

[Kai Engert (:KaiE:)]

trunk:

Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.85; previous revision: 1.84
done
Checking in certhigh/certvfypkix.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v  <--  certvfypkix.c
new revision: 1.47; previous revision: 1.46
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.213; previous revision: 1.212
done

Comment 15

[Kai Engert (:KaiE:)]

3.12.10:

Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.80.2.3; previous revision: 1.80.2.2
done
Checking in certhigh/certvfypkix.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v  <--  certvfypkix.c
new revision: 1.46.2.1; previous revision: 1.46
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.206.2.6; previous revision: 1.206.2.5
done