Bug #78058 recv_parse_log_rec violates its contract re. incomplete recs for MLOG_CHECKPOINT
Submitted: 13 Aug 2015 16:21 Modified: 17 Aug 2015 14:17
Reporter: Laurynas Biveinis Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: InnoDB storage engine Severity:S3 (Non-critical)
Version:5.7 OS:Any
Assigned to: CPU Architecture:Any
Tags: code quality, log, mlog_checkpoint, mtr, recovery

[13 Aug 2015 16:21] Laurynas Biveinis
Description:
recv_parse_log_rec is documented as follows:

** Tries to parse a single log record.
...
@param[in]      ptr             pointer to a buffer
@param[in]      end_ptr         end of the buffer
...
@return length of the record, or 0 if the record was not complete */

The record is complete if its length <= end_ptr - ptr, in which case length is returned, otherwise the record is incomplete and zero is returned. This is true for all the record types except for MLOG_CHECKPOINT, for which SIZE_OF_MLOG_CHECKPOINT is returned regardless of end_ptr - ptr. Then the caller handles the incomplete record case itself. This could be a problem for patches that reuse recv_parse_log_rec for something else.

A uniform incomplete record handling regardless of the record type would be cleaner.

This state of things could be a result of MLOG_CHECKPOINT being one-byte record originally, and handled as other one byte records in this function.

How to repeat:
Code analysis.

Suggested fix:
@@ -2338,7 +2349,8 @@ recv_parse_log_rec(
                return(1);
        case MLOG_CHECKPOINT:
                *type = static_cast<mlog_id_t>(*ptr);
-               return(SIZE_OF_MLOG_CHECKPOINT);
+               return ((end_ptr - ptr < SIZE_OF_MLOG_CHECKPOINT)
+                       ? 0 : SIZE_OF_MLOG_CHECKPOINT);
        case MLOG_MULTI_REC_END | MLOG_SINGLE_REC_FLAG:
        case MLOG_DUMMY_RECORD | MLOG_SINGLE_REC_FLAG:
        case MLOG_CHECKPOINT | MLOG_SINGLE_REC_FLAG:
@@ -2563,9 +2575,6 @@ loop:
                        /* Do nothing */
                        break;
                case MLOG_CHECKPOINT:
-                       if (end_ptr < ptr + SIZE_OF_MLOG_CHECKPOINT) {
-                               return(false);
-                       }
 #if SIZE_OF_MLOG_CHECKPOINT != 1 + 8
 # error SIZE_OF_MLOG_CHECKPOINT != 1 + 8
 #endif
[14 Aug 2015 14:31] MySQL Verification Team
Hi,

Thank you for your bug report. Can you confirm something for me.

This is actually not a report about a bug, but a request that function recv_parse_log_rec() always returns the results of the same meaning for all record types , so that records of the type MLOG_CHECKPOINT  do not return always SIZE_OF_MLOG_CHECKPOINT, but the real length. Is this what you are requesting ???

If you confirm your request than we will have to check whether a design of this function is intended to be inconsistent or not.
[14 Aug 2015 14:36] Laurynas Biveinis
Yes, that is my request (except that instead "do not return always SIZE_OF_MLOG_CHECKPOINT, but the real length" it should say "return SIZE_OF_MLOG_CHECKPOINT only if there are at least that many bytes in the buffer available, zero otherwise", because MLOG_CHECKPOINT is a fixed-size record).

In the case this inconsistency is by design, the bug should become about an incorrect function header comment (which should then describe exceptional MLOG_CHECKPOINT handling explicitly).
[17 Aug 2015 6:00] Marko Mäkelä
Hi Laurynas, thanks for the report! I am glad that someone is reading my code. :)

You are right, it looks like I forgot to adjust this piece of code when I changed the size of the MLOG_CHECKPOINT record so that it would include the LSN of the checkpoint that it is referring to.
[17 Aug 2015 14:17] Daniel Price
Posted by developer:
 
Fixed as of the upcoming 5.7.9, 5.8.0 releases, and here's the changelog entry:

The recv_parse_log_rec function returned the length of the redo log
record instead of 0 when encountering an incomplete MLOG_CHECKPOINT
record. 

Thank you for the bug report.