[xmlsec] some changes for NSS support, some more pending

[Tejkumar Arora]

Hi Aleksey,
Thanks for the code review.... I'll fix these issues
and send you another patch, which will include some
more changes... (pkcs12...).

-Tej

Aleksey Sanin wrote:

 > Hi, Tej!
 >
 > I did a first pass thru your code and you can find my comments bellow. I
 > created
 > a new branch "XMLSEC_NSS_030714" and checked in this patch along with
 > my changes. I would suggest to use this branch till we clear all the
 > legal and technical
 > issues with xmlsec-nss.
 >
 > BTW, have you tried "make memcheck" (you need valgrind installed)? It
 > seems like
 > there might be some memory leaks in this code.
 >
 > Aleksey
 >
 > 0) General comments on style:
 >     - Please check the warnings! For example, you did not have "#include
 > <xmlsec/nss/pkikeys.h>"
 >     in src/nss/pkikeys.c and "#include <xmlsec/nss/bignum.h>" in
 > src/nss/bignum.c
 >     This might create problems for some platforms. In best case, you
 > should have zero
 >     warnings during compilation with "--enable-pedantic" configurure.in
 > option.
 >     I cleared some of the warnings but there are a lot more to do.
 >     - Please put explicit != NULL, != 0, etc. This makes code more easy
 > to read.
 >     - Please put figure brackets {} even if you have only one operator.
 >     For example
 >           if(aaa)
 >                 xmlFree(aaa);
 >     should be
 >           if(aaa != NULL) {
 >                 xmlFree(aaa);
 >           }
 >     - Please put round brackets for "return" like "return(0)"
 >     - Please round brackets in conditions like:
 >           if(privkey == NULL || pubkey == NULL)
 >       should be
 >           if((privkey == NULL) || (pubkey == NULL))
 >
 >
 > 1) GetFileContents() (src/nss/app.c) should probably be renamed to
 > something
 >   like xmlSecNssAppReadSECItem()
 > 2) xmlSecNssAppKeyLoad(src/nss/app.c)
 >        - xmlSecKeyDataPtr data; is not initialized and you do if(data)
 > after goto;
 >        probably it's better to add "= NULL".
 >        - "memset(&filecontent, 0, sizeof(SECItem));" IMHO, it's better
 > to use variable
 >        name in this case: memset(&filecontent, 0, sizeof(filecontent));
 >        - use "done" instead of "out" goto label
 > 3) xmlSecNSSPKIKeyDataCtxDup() (src/nss/pkikeys.c):
 >     - Don't use xmlSecAssert() to check result of copy operation; it's
 > an error
 >     if copy returns NULL, not an assert
 > 4) xmlSecNssKeyDataDsaXmlRead() and xmlSecNssKeyDataRsaXmlRead()
 > (src/nss/pkikeys.c):
 >     - "data" might be used w/o initialization
 >     - The following code looks wrong to me:
 >     if (ret == 0) {
 >         return ret;
 >     }
 >     if (pubkey != NULL) {
 >         SECKEY_DestroyPublicKey(pubkey);
 >     }
 >     if (data != NULL) {
 >         xmlSecKeyDataDestroy(data);
 >     }
 >     return (ret);
 >
 >     We either should always destroy "pubkey" (if it's not a part of
 > "data") or we should never
 >     destroy it (if it's a part of "data").
 >
 >     It would also great if you can avoid doing such "double returns".
 > This is very difficult to read
 >     and I bet that next time someone touch this code there would be an
 > error because of missed
 >     first return.
 >
 > 5) xmlSecNssX509FindCert() (src/nss/x509vfy.c):
 >        - I really don't like the code structure here: several goto
 > labels, etc. May be it would be better
 >     to split it in separate functions?
 > 6) xmlSecNssNodeSetBigNumValue() (src/nss/bignum.c):
 >        - The "size" variable was used uninitialized, you actually don't
 > need it there.
 >
 >
 >
 >
 >
 >
 >
 > _______________________________________________
 > xmlsec mailing list
 > xmlsec at aleksey.com
 > http://www.aleksey.com/mailman/listinfo/xmlsec