Bug #82734 trx_undo_rec_copy needlessly relies on buffer pool page alignment
Submitted: 25 Aug 2016 23:18 Modified: 10 Dec 2019 19:11
Reporter: David Gow (OCA) Email Updates:
Status: Closed 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 2019 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 2019 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 2019 17:13] Sinisa Milivojevic
Thank you for your analysis.

This is now a fully verified feature request.

Thank you for your contribution.
[6 Mar 2019 15:09] Sinisa Milivojevic
Upgrading the severity.
[10 Dec 2019 19:11] Daniel Price
Posted by developer:
 
Fixed as of the upcoming 5.7.30, 8.0.20 releases, and here's the changelog entry:

A function that calculates undo log record size could calculate an
incorrect length value in the case of a corrupted undo log record,
resulting in a malloc failure. Assertion code was added to detect
incorrect calculations.
[11 Dec 2019 13:42] Sinisa Milivojevic
Thank you, Daniel.