Bug #82734 trx_undo_rec_copy needlessly relies on buffer pool page alignment
Submitted: 25 Aug 2016 23:18 Modified: 5 Mar 17:13
Reporter: David Gow (OCA) Email Updates:
Status: Verified Impact on me:
None 
Category:MySQL Server: InnoDB storage engine Severity:S2 (Serious)
Version:5.7, 8.0 OS:Any
Assigned to: CPU Architecture:Any

[25 Aug 2016 23:18] David Gow
Description:
The trx_undo_rec_copy() function could do with some tidying, as its implementation can be quite confusing:
https://github.com/mysql/mysql-server/blob/5.7/storage/innobase/include/trx0rec.ic#L93

Most notably, by calculating the pointer to the beginning of the page using ut_align_offset():
- it relies on the pointer to an undo log entry (undo_rec) coming from a page in memory which is aligned on a PAGE_SIZE boundary. This is the case with the buffer pool, but is not explicitly documented.
- in the event of the undo log entry being invalid (for example, the offset to the next record being > PAGE_SIZE), it often fails by attempting to allocate a huge amount of memory (typically ~2^64 EiB). Ideally this would be (at least in debug builds) an assertion.
- the code is difficult to understand, and fragile when its assumptions are broken.

How to repeat:
Code inspection at:
https://github.com/mysql/mysql-server/blob/5.7/storage/innobase/include/trx0rec.ic#L93

Suggested fix:
Rework the function:
- Accept a pointer to the start of the page. Most callers of the function already have such a pointer present. Use this instead of relying on the page being aligned in memory.
- Assert that the offset to next undo record is sane.
- Ideally, comment the function to specify that the length is calculated by subtracting the current offset from the offset to the next undo log entry.

Thanks!
[5 Mar 12:54] Sinisa Milivojevic
Hi,

Your comments are very useful.

I truly find them justified, by inspecting the code.

However, 5.7 is a very stable version, highly unlikely (although not impossible) to get improvements, such as you suggest. 

Could you please, also, analyse the same code in 8.0 and let us know whether your suggestions still apply ???

Many thanks in advance.
[5 Mar 16:27] Bernardo Perez
Hello Sinisa,

I did checked the code for trx0rec.ic#L93 in 8.0.12

 86 /** Copies the undo record to the heap.
 87  @return own: copy of undo log record */
 88 UNIV_INLINE
 89 trx_undo_rec_t *trx_undo_rec_copy(
 90     const trx_undo_rec_t *undo_rec, /*!< in: undo log record */
 91     mem_heap_t *heap)               /*!< in: heap where copied */
 92 {
 93   ulint len;
 94
 95   len = mach_read_from_2(undo_rec) - ut_align_offset(undo_rec, UNIV_PAGE_SIZE);
 96   ut_ad(len < UNIV_PAGE_SIZE);
 97   return ((trx_undo_rec_t *)mem_heap_dup(heap, undo_rec, len));
 98 }
 99 #endif /* !UNIV_HOTBACKUP */

Same as in 5.7:
 83
 84 /***********************************************************************//**
 85 Copies the undo record to the heap.
 86 @return own: copy of undo log record */
 87 UNIV_INLINE
 88 trx_undo_rec_t*
 89 trx_undo_rec_copy(
 90 /*==============*/
 91         const trx_undo_rec_t*   undo_rec,       /*!< in: undo log record */
 92         mem_heap_t*             heap)           /*!< in: heap where copied */
 93 {
 94         ulint           len;
 95
 96         len = mach_read_from_2(undo_rec)
 97                 - ut_align_offset(undo_rec, UNIV_PAGE_SIZE);
 98         ut_ad(len < UNIV_PAGE_SIZE);
 99         return((trx_undo_rec_t*) mem_heap_dup(heap, undo_rec, len));
100 }
101 #endif /* !UNIV_HOTBACKUP */
[5 Mar 17:13] Sinisa Milivojevic
Thank you for your analysis.

This is now a fully verified feature request.

Thank you for your contribution.
[6 Mar 15:09] Sinisa Milivojevic
Upgrading the severity.