[MDEV-12634] Uninitialised ROW_MERGE_RESERVE_SIZE bytes written to temporary file Created: 2017-04-29 Updated: 2017-09-28 Resolved: 2017-09-14 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Storage Engine - InnoDB, Storage Engine - XtraDB |
| Affects Version/s: | 10.1.7, 10.2.0, 10.3.0 |
| Fix Version/s: | 10.1.27, 10.2.9 |
| Type: | Bug | Priority: | Major |
| Reporter: | Elena Stepanova | Assignee: | Jan Lindström (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Issue Links: |
|
||||||||||||||||||||
| Sprint: | 10.1.24 | ||||||||||||||||||||
| Description |
|
|
| Comments |
| Comment by Elena Stepanova [ 2017-08-10 ] | ||||||||||||||||||||||
|
Reproducible locally (Jessie 8.8 64-bit, valgrind-3.12.0.SVN, gcc (Debian 4.9.2-10) 4.9.2) on the current 10.1 tree:
| ||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-08-10 ] | ||||||||||||||||||||||
|
This is due to the ROW_MERGE_RESERVE_SIZE (4) bytes at the start of the first block of the merge sort file. This bug was introduced in MariaDB 10.1.7 by I do not understand why we need to allocate and write these bytes to the file even when using encryption. Running vgdb instead of or inside manual-gdb confirms this:
The only uninitialized bytes are the first 4 bytes of the buffer. | ||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-08-10 ] | ||||||||||||||||||||||
|
When fixing this (by removing the ROW_MERGE_RESERVE_SIZE), please also ensure that row_log_apply() and row_log_table_apply() are using encryption on the temporary files. | ||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-09-08 ] | ||||||||||||||||||||||
|
https://github.com/MariaDB/server/commit/6550bae39d848e0359df44ea5a85cf0d574dd5c3 | ||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-09-08 ] | ||||||||||||||||||||||
|
Fix is quite big as we need to pass key_version to several functions and there really is not any suitable structure where it could be stored. Note that you can't use min_key_version or key_found in crypt_data structure as they could change during alter table (background key rotation could change them). | ||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-09-08 ] | ||||||||||||||||||||||
|
I would like to have some simpler data structures. | ||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-09-08 ] | ||||||||||||||||||||||
|
My suggestion would be to introduce fil_space_t::key_version. When ALTER TABLE…ALGORITHM=INPLACE is in progress (creating indexes or rebuilding a table), I think that key rotation for the affected tablespace can be disabled. | ||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-09-13 ] | ||||||||||||||||||||||
|
Requesting a new review as implementation changed significantly: | ||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-09-13 ] | ||||||||||||||||||||||
|
I think that the patch can be simplified further. We can simply make the encryption of the temporary files dependent on the innodb_encrypt_log parameter. | ||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-09-14 ] | ||||||||||||||||||||||
|
commit fa2701c6f7b028782cf231565f578b2fc0f10d51 Fixed by removing writing key version to start of every block that After this MDEV also blocks writen to row log are encrypted and blocks innodb_status_variables[], struct srv_stats_t Removed ROW_MERGE_RESERVE_SIZE define. row_merge_fts_doc_tokenize row_log_t row_log_online_op, row_log_table_close_func row_log_table_apply_ops, row_log_apply_ops row_log_allocate row_log_free row_merge_encrypt_buf, row_merge_decrypt_buf row_merge_buf_create, row_merge_buf_write row_merge_build_indexes log_tmp_blocks_crypt, log_tmp_block_encrypt, log_temp_block_decrypt log_tmp_is_encrypted Added test case innodb-rowlog to force creating a row log and | ||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-09-15 ] | ||||||||||||||||||||||
|
Please push a merge of the 10.1 commit and 10.2 to a staging branch, for review. There are lots of conflicts. | ||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-09-15 ] | ||||||||||||||||||||||
|
I would revert this instrumentation from 10.1 and instead, introduce an innodb_encrypt_log.combinations file and use it in the tests innodb.innodb-table-online and innodb.innodb-index-online.
Those two tests use proper DEBUG_SYNC synchronization and do not introduce such unreasonable (and inherently nondeterministic) delays. | ||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-09-17 ] | ||||||||||||||||||||||
|
I pushed the merge to 10.2 and an adjustment to innodb-table-online that makes it use the merge sort files. | ||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-09-18 ] | ||||||||||||||||||||||
|
greenman Can you update the documentation to mention that in 10.1.27, 10.2.9,10.3.2 temporary files created by merge sort and row log are encrypted if innodb-encrypt-log=TRUE regardless is table encrypted or not. | ||||||||||||||||||||||
| Comment by Ian Gilfillan [ 2017-09-18 ] | ||||||||||||||||||||||
|
jplindst so this is regardless of the setting of encrypt_tmp_files, correct? | ||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-09-18 ] | ||||||||||||||||||||||
|
Correct, this effects only temporary files created and used by InnoDB internally. |