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 [EMAIL PROTECTED] http://www.aleksey.com/mailman/listinfo/xmlsec
