Hi, Jan! On Mar 13, Jan Lindström wrote:

revision-id: 7c21ea9f502ded155c12a0ee3c5ff0602e9d4c9a parent(s): 4d0e52189ca945c94146035c2733d85c9c6fd18d committer: Jan Lindström branch nick: 10.1-innodb timestamp: 2015-03-13 14:18:07 +0200 message:

MDEV-7772: SIGSEGV on my_aes_encrypt_cbc when -DWITH_SSL=bundled

Two problems: - Read/Write outside of buffer at memcpy() because of incorrect parameters . OPENSSL_assert(EVP_CIPHER_CTX_iv_length(&ctx.ctx) == iv_length); // ECB does not use IV, thus incorrect assertion

Added: mysql-test/include/have_file_key_management_plugin.combinations ( to run with aes_cbc and aes_ecb) mysql-test/include/have_openssl_ctr.combinations ( to run with aes_cbc, aes_ecb and aes_ctr)

The patch is ok, of course. Just a few suggestions, see below. Ok to push after that.

diff --git a/mysql-test/include/have_file_key_management_plugin.combinations b/mysql-test/include/have_file_key_management_plugin.combinations new file mode 100644 index 0000000..6a63b5a --- /dev/null +++ b/mysql-test/include/have_file_key_management_plugin.combinations @@ -0,0 +1,6 @@ +[aes_cbc] +encryption-algorithm=aes_cbc + +[aes_ecb] +encryption-algorithm=aes_ecb +

1. Does file_key_management_plugin work with CTR? 2. suggestion: use shorter names for combinations (I'd remove "aes_" prefix) test names are long enough already. See replication combinations - they are 3-4 letters only (rbr/mix/stmt).

diff --git a/mysql-test/include/have_openssl_ctr.combinations b/mysql-test/include/have_openssl_ctr.combinations new file mode 100644 index 0000000..2c49c78 --- /dev/null +++ b/mysql-test/include/have_openssl_ctr.combinations @@ -0,0 +1,9 @@ +[aes_cbc] +encryption-algorithm=aes_cbc + +[aes_ecb] +encryption-algorithm=aes_ecb + +[aes_ctr] +encryption-algorithm=aes_ctr +

This doesn't make a lot of sense now, because example_key_management_plugin forces CTR: my_aes_init_dynamic_encrypt(MY_AES_ALGORITHM_CTR); so it always uses ctr, no matter what encryption-algorithm is. which is very confusing, SHOW VARIABLES will show, say, aes_ecb, but in fact it will be aes_ctr under the hood. furthermore, it doesn't check for the return value (so it'll fail later or will misbehave on old openssl versions that don't support CTR). This all is a separate bug (in Jira already), but for now I'm just saying that it's kind of pointless to run example_key_management_plugin with different encryption-algorithm values.

diff --git a/mysql-test/suite/innodb/t/innodb_scrub_background.test b/mysql-test/suite/innodb/t/innodb_scrub_background.test index 44cb16b..881c124 100644 --- a/mysql-test/suite/innodb/t/innodb_scrub_background.test +++ b/mysql-test/suite/innodb/t/innodb_scrub_background.test @@ -1,6 +1,7 @@ -- source include/have_innodb.inc -- source include/not_embedded.inc -- source include/have_example_key_management_plugin.inc +-- source include/have_openssl_ctr.inc

that's fine, although you could've simply included have_openssl_ctr.inc from have_example_key_management_plugin.inc :)

let $MYSQLD_DATADIR=`select @@datadir`; let ib1_IBD = $MYSQLD_DATADIR/ibdata1; diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/fil0crypt.cc index 823c74a..10d49bb 100644 --- a/storage/innobase/fil/fil0crypt.cc +++ b/storage/innobase/fil/fil0crypt.cc @@ -734,7 +734,7 @@ fil_space_encrypt(ulint space, ulint offset, lsn_t lsn, uint32 dstlen;

if (page_compressed) { - srclen = page_size - FIL_PAGE_DATA;; + srclen = page_size - FIL_PAGE_DATA;

I'd suggest to move this and all following changes to a separate commit.

}

int rc = (* my_aes_encrypt_dynamic)(src, srclen,

Regards, Sergei