| 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 (OCA) | Email Updates: | |
| Status: | Closed | Impact on me: | |
| 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 | ||
[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.
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