Code review comment for lp:~laurynas-biveinis/percona-server/merge-5.6.17

> Changes to lock_rec_other_trx_holds_expl() look okay-ish, though they
> could benefit from the trx descriptors optimization by replacing
> pointers to transactions with their IDs. I don’t insist, since it’s a
> debug-only code.

Debug-only code was the reason why I did not do this the first time
around, but why not. Does the following look reasonable?

 lock_mutex_enter();
 mutex_enter(&trx_sys->mutex);

 trx_id_t* impl_trx_desc = trx_find_descriptor(trx_sys->descriptors,
            trx_sys->descr_n_used,
            trx_id);
 if (impl_trx_desc) {
  trx_id_t impl_trx_id = *impl_trx_desc;
  ulint heap_no = page_rec_get_heap_no(rec);
  ulint rw_trx_count = trx_sys->descr_n_used;
  trx_id_t* rw_trx_snapshot = static_cast<trx_id_t *>
   (ut_malloc(sizeof(trx_id_t) * rw_trx_count));
  memcpy(rw_trx_snapshot, trx_sys->descriptors,
         sizeof(trx_id_t) * rw_trx_count);

  mutex_exit(&trx_sys->mutex);

  for (ulint i = 0; i < rw_trx_count; i++) {

   mutex_enter(&trx_sys->mutex);
   trx_t* t = trx_rw_get_active_trx_by_id(
    rw_trx_snapshot[i], NULL);
   mutex_exit(&trx_sys->mutex);

   lock_t* expl_lock = lock_rec_has_expl(precise_mode,
             block, heap_no,
             t);
   if (expl_lock && expl_lock->trx->id != impl_trx_id) {
    /* An explicit lock is held by trx other than
    the trx holding the implicit lock. */
    holds = expl_lock->trx;
    break;
   }
  }

  ut_free(rw_trx_snapshot);

 } else {
  mutex_exit(&trx_sys->mutex);
 }

 lock_mutex_exit();

> But the OpenSSL changes are wrong. It looks like the only issue with
> OpenSSL < 0.9.8l that breaks AES_ENCRYPT/AES_DECRYPT compatibility is
> that those old versions incorrectly report the required IV length with
> AES in ECB mode. IV is not required by the ECB cipher, so
> EVP_CIPHER_iv_length() should return 0, whereas old versions
> return 16.

Yes.

> Which confuses my_aes_needs_iv(), in particular, and makes the server
> believe AES_*() functions require the 3rd argument with
> block_encryption_mode=aes-128-ecb (i.e. an IV).

Yes.

> Which is also an upstream issue, i.e. a workaround for old OpenSSL
> libs is required. And the workaround is simple: whenever
> EVP_CIPHER_iv_length() is used to get the required IV length, also
> check EVP_CIPHER_mode(). If it’s EVP_CIPH_ECB_MODE, disregard the
> value returned by EVP_CIPHER_iv_length() and assume the
> required IV length to be 0.

OK, that's a better workaround than mine.

> The approach implemented in this MP is rather dangerous: we make the
> server pretend that it accepts IV when required, when in fact it is
> ignored for old OpenSSL versions. Security folks may get upset :)

My idea was to make MySQL call OpenSSL in exactly same way (Foo vs
FooEx aside) it was in 5.6.16-, including 5.5 GA etc. So that
implementation would be as secure or insecure as, say, current 5.5.

> Speaking of the new Openssl_version variable, its only purpose in this
> MP was to patch up the test suite to hide the fact that IV is ignored
> for old OpenSSL versions. Which means there’s no need for it
> anymore.

Right, removing.

> Besides, it is misleading: it shows the OpenSSL version that
> the server was built against. Which, in case of dynamic linking may
> differ from the OpenSSL library actually being used. Which can make
> quite a difference in the light of recent events.

I wanted to make it nothing more than a build OpenSSL version for API
versioning purposes, not a runtime version indicating a
patchlevel. It implements this well IMHO, but is horribly misnamed as
such and potential for misuse is very high.