[webkit-reviews] review denied: [Bug 27543] Add platform/wince/ files for WINCE port : [Attachment 34381] 7) Keygen (Security)

[bugzilla-daemon at webkit.org]

George Staikos <staikos at kde.org> has denied Yong Li <yong.li at torchmobile.com>'s
request for review:
Bug 27543: Add platform/wince/ files for WINCE port
https://bugs.webkit.org/show_bug.cgi?id=27543

Attachment 34381: 7) Keygen (Security)
https://bugs.webkit.org/attachment.cgi?id=34381&action=review

------- Additional Comments from George Staikos <staikos at kde.org>
For some meaningful review,

>> +#include "config.h"
> +#include "SSLKeyGenerator.h"
> +
> +#include "Base64.h"
> +#include "CString.h"
> +
> +#include <windows.h>
> +#include <wincrypt.h>

Is this out of order by design?

> +
> +#define KEYGEN_DEBUG_KEY	   0x00000001
> +
> +#ifndef NDEBUG
> +#define KEYGEN_DEBUG    (0)
> +#else
> +#define KEYGEN_DEBUG    0
> +#endif

This is odd.  Why 0 or 0?  I think we need to do better on the debug scheme.

> +String WebCore::signedPublicKeyAndChallengeString(unsigned index, const
String& challenge, const KURL& url)
> +{
> +    String keystr;
> +
> +    HCRYPTPROV hProv = 0;
> +    HCRYPTKEY hKey = 0;
> +    PCERT_PUBLIC_KEY_INFO pPubInfo = 0;
> +    DWORD dwPubInfoLength = 0;
> +    CERT_KEYGEN_REQUEST_INFO requestInfo;
> +    CRYPT_ALGORITHM_IDENTIFIER signAlgo;
> +    LPBYTE pbEncoded = 0;
> +    DWORD dwEncodedLength = 0;
> +    LPWSTR pszString = 0;
> +    Vector<char> binary;
> +    Vector<char> base64;

A good number of these can be moved to the relevant area so we don't lose track
of what they're used for and if they're no-longer used.

> +    BOOL success = CryptAcquireContext(&hProv, _T("keygen_container"),
MS_ENHANCED_PROV, PROV_RSA_FULL, CRYPT_NEWKEYSET);
> +    if (!success)
> +	   goto error;

   Can we do better than lots of gotos?

> +    pPubInfo = (PCERT_PUBLIC_KEY_INFO) new char[dwPubInfoLength];
> +    if (!pPubInfo)
> +	   goto error;

  No need to check new return.

> +    if (challenge.length()) {
> +	   pszString = new WCHAR[challenge.length() + 1];
> +	   if (pszString) {

   Same.

> +    pbEncoded = (LPBYTE) new char[dwEncodedLength];
> +    if (!pbEncoded)
> +	   goto error;

   Same

> +error:
> +    if (pszString)
> +	   delete[] pszString;
> +    if (pbEncoded)
> +	   delete[] pbEncoded;
> +    if (pPubInfo)
> +	   delete[] pPubInfo;

   Those if() are redundant