MySQL Bugs: #82734: trx_undo_rec_copy needlessly relies on buffer pool page alignment

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	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

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!

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.

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 */

Thank you for your analysis.

This is now a fully verified feature request.

Thank you for your contribution.

Upgrading the severity.

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.

Thank you, Daniel.