Re: pgcrypto compilation error due to stack-allocated EVP_CIPHER_CTX

On Fri, Dec 9, 2016 at 1:11 AM, Asif Naeem <anaeem(dot)it(at)gmail(dot)com> wrote:
> It make sense. I would like to share more comments as following i.e.
>
>> static int
>> bf_check_supported_key_len(void)
>> {
>> ...
>> /* encrypt with 448bits key and verify output */
>> evp_ctx = EVP_CIPHER_CTX_new();
>> if (!evp_ctx)
>> return 1;
>> if (!EVP_EncryptInit_ex(evp_ctx, EVP_bf_ecb(), NULL, NULL, NULL))
>> goto leave;
>> if (!EVP_CIPHER_CTX_set_key_length(evp_ctx, 56))
>> goto leave;
>> if (!EVP_EncryptInit_ex(evp_ctx, NULL, NULL, key, NULL))
>> goto leave;
>> if (!EVP_EncryptUpdate(evp_ctx, out, &outlen, data, 8))
>> goto leave;
>> if (memcmp(out, res, 8) != 0)
>> goto leave; /* Output does not match ->
>> strong cipher is
>> * not supported */
>> status = 1;
>> leave:
>> EVP_CIPHER_CTX_free(evp_ctx);
>> return status;
>> }
>
>
> It seems that it need to return 0 instead of 1 in case of failure i.e.

Yep that's wrong. Thanks for pointing that out.

>> /* encrypt with 448bits key and verify output */
>> evp_ctx = EVP_CIPHER_CTX_new();
>> if (!evp_ctx)
>> return 0;
>
> We can avoid multiple if conditions and goto statement something like i.e.
>
>> if (EVP_EncryptInit_ex(evp_ctx, EVP_bf_ecb(), NULL, NULL, NULL) &&
>> EVP_CIPHER_CTX_set_key_length(evp_ctx, 56) &&
>> EVP_EncryptInit_ex(evp_ctx, NULL, NULL, key, NULL) &&
>> EVP_EncryptUpdate(evp_ctx, out, &outlen, data, 8) &&
>> memcmp(out, res, 8) == 0 )) /* Output does not match -> strong
>> cipher is not supported */
>> status = 1;
>> EVP_CIPHER_CTX_free(evp_ctx);
>> return status;
>> }

I thought about doing that as well to be honest :) One way or the
other is fine, still I recall seeing more the style of the current
patch in PG code though, and that sticks better with current HEAD. But
my impressions may be wrong.

> What is your opinion ?. I am hopeful I will be able to share all my findings
> tomorrow. Thanks.

Thanks for looking at the patch. Looking forward to hearing more!
--
Michael