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

[Aleksey Sanin]

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.