Hi all, Thanks for the feedback. I was mostly concerned with the happy path at first, fixing memory management was on my list, but given it's the recurring comment, I am starting on that now. This is my first real C project, so your criticism is welcome.
Regarding the API, I can't simply expose the same API as the definition, since at least, there won't be the need to pass the various argument lengths in PHP land. Since I was going to change the API anyway, I thought it best to use the recommended code convention for class and method names. It is also in line with other languages PKCS11 implementations. One compromise I think is appropriate would be to provide the vanilla API as functions (as they are) taking a Pkcs11\Session object as the session handler. and leave it up to the user to choose how to use it. @vjardin, what do you think ? Regarding C_GetAttributeValue() return code, this is more of a design choice, since I absolutely despise passing a variable reference to be filled by the method, returning a partial result, along with an explanation would require some hoop jumping. Not impossible, but worth thinking it over. What I see is returning a new AttributeSet object, with isComplete(): bool (true if CKR_OK, false otherwise), and getError() returning the code. Not married to it though, I am open to suggestions. https://python-pkcs11.readthedocs.io/en/latest/applied.html https://godoc.org/github.com/miekg/pkcs11 Thank you for taking the time to comment. *Guillaume Amringerhttps://keybase.io/gamr <https://keybase.io/gamr>https://www.linkedin.com/in/guillaumeamringer <https://www.linkedin.com/in/guillaumeamringer>https://github.com/gamringer <https://github.com/gamringer>* On Mon, Jul 20, 2020 at 3:46 PM <vjar...@free.fr> wrote: > Hi Guillaume, > > same comments than Johannes, ecalloc()/efree()/emalloc() should be used. > Since they are many crypto sensitive contents, only ecalloc() should be > used (no emalloc()). > > Regarding the Oasis' semantic, I feel that it is important to keep it "as > is" so the vanilla documentation would be the upstream Oasis PKCS11 > standard. > It means, for instance: > getInfo() -> C_GetInfo() > > It is nice to have some sign() methods in order to avoid combining > C_SignInit() + C_Sign(), but they are some cases when we need to stick with > the Oasis' specifications, so I believe that > C_SignInit() > C_VerifyInit() > C_FindObjectsInit() > C_FindObjectsFinal() > etc. > should still be exposed per the Oasis' specification, along having some > nice sign(), verify(), etc. methods too. > > Regarding: > C_GetAttributeValue() > it has many "OK" cases: > See Oasis' comments: > "Note that the error codes CKR_ATTRIBUTE_SENSITIVE, > CKR_ATTRIBUTE_TYPE_INVALID, and CKR_BUFFER_TOO_SMALL do not denote true > errors for C_GetAttributeValue. If a call to C_GetAttributeValue returns > any of these three values, then the call MUST nonetheless have processed > every attribute in the template supplied to C_GetAttributeValue. Each > attribute in the template whose value can be returned by the call to > C_GetAttributeValue will be returned by the call to C_GetAttributeValue." > > my 2 cents, > Vincent > > ----- Mail original ----- > De: "Johannes Schlüter" <johan...@schlueters.de> > À: "Guillaume Amringer" <g.amrin...@gmail.com>, "pecl-dev" < > pecl-dev@lists.php.net> > Envoyé: Lundi 20 Juillet 2020 14:55:33 > Objet: Re: [PECL-DEV] PKCS11 Extension > > Hi, > > On Sun, 2020-07-19 at 22:33 -0400, Guillaume Amringer wrote: > > The extension is currently hosted here: > > https://github.com/gamringer/php-pkcs11 > > I scrolled over it without much attention (thus probably missed other > issues) a few comments to this line I noticed: > > pSlotList = (CK_SLOT_ID_PTR) malloc(ulSlotCount * sizeof(CK_SLOT_ID)); > https://github.com/gamringer/php-pkcs11/blob/master/pkcs11.c#L300 > > 1. The return value of malloc() is not checked. If the system runs out > of memory (OOM) or for some other reason can't return a memory block > this will return NULL and then lead to undefined behavior later. > (unlikely on today's systems, but might happen) > 2. emalloc should be used instead of malloc. For one it counts towards > PHP's memory_limit, ten it also ensures the memory is freed if > something weird happens by the end of the request, thus reduces risk > of memory leaks and it fixes point 1. by terminating the request in > an OOM situation. > 3. ulSlotCount * sizeof(CK_SLOT_ID) could eventually overflow. Better > use > safe_emalloc(ulSlotCount, sizeof(CK_SLOT_ID), 0) > this > calculates > ulSlotCount * sizeof(CK_SLOT_ID) + 0 > in a overflow-safe > way and errors out in case of a problem. > > johannes > > -- > PECL development discussion Mailing List (https://pecl.php.net/) > To unsubscribe, visit: https://www.php.net/unsub.php >
