Binlog JSON Parser: handle inline types correctly for large documents #8187
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e detail in binlog parsing to identify errant data Signed-off-by: Rohit Nayak <rohit@planetscale.com>
rohit-nayak-ps
changed the title
rohit-nayak-ps
requested review from
harshit-gangal,
shlomi-noach and
systay
as code owners
…ensive comments. Add test for reported failure mode and improve other tests Signed-off-by: Rohit Nayak <rohit@planetscale.com>
rohit-nayak-ps
force-pushed
the
rn-json-data-panic
branch
from
4cc1b75
to
225df50
Compare
rohit-nayak-ps
changed the title
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
rohit-nayak-ps
changed the title
|
This is now ready for review: applying a patch with the inline fix in this PR fixed the MoveTables workflow that was failing. |
shlomi-noach
reviewed
Jun 2, 2021
| } else { | ||
| fmt.Printf("%02d ", c) | ||
| s += fmt.Sprintf("%02d ", c) |
There was a problem hiding this comment.
This concatenation will end up inefficient. Consider using strings.Builder
There was a problem hiding this comment.
Actually this is only used for local debugging of the functionality. It is off in production. It will be removed soon once we have a few more prod tests with the json data type.
There was a problem hiding this comment.
ah, cool. In that case, keep it as it is.
| } | ||
|
|
||
| // only used for logging/debugging | ||
| var jsonTypeToName = map[uint]string{ |
There was a problem hiding this comment.
is there no boolean type?
There was a problem hiding this comment.
the literal type is used for booleans and nulls
Comment on lines
+259
to
+292
| return length, pos | ||
| } | ||
|
|
||
| // getElem returns the json value found inside json objects and arrays at the provided position | ||
| func getElem(data []byte, pos int, large bool) (*ajson.Node, int, error) { | ||
| var elem *ajson.Node | ||
| var err error | ||
| var offset int | ||
| typ := jsonDataType(data[pos]) | ||
| pos++ | ||
| if isInline(typ, large) { | ||
| elem, err = binlogJSON.getNode(typ, data, pos) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
| if large { | ||
| pos += 4 | ||
| } else { | ||
| pos += 2 | ||
| } | ||
| } else { | ||
| offset, pos = readInt(data, pos, large) | ||
| if offset >= len(data) { // consistency check, should only come here is there is a bug in the code | ||
| log.Errorf("unable to decode element") | ||
| return nil, 0, fmt.Errorf("unable to decode element: %+v", data) | ||
| } | ||
| newData := data[offset:] | ||
| //newPos ignored because this is an offset into the "extra" section of the buffer | ||
| elem, err = binlogJSON.getNode(typ, newData, 1) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
| } | ||
| return elem, pos, nil |
There was a problem hiding this comment.
gonna trust you on this 😅
shlomi-noach
approved these changes
Jun 2, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In JSON Objects and Arrays some data types are inlined. The existing code only expected that 2 byte values (int16s, literals, null) could be inlined. However for "large" documents (> 64K) mysql also inlines 4 byte values (int32s). This resulted in out-of-bounds panics because the inline 32-bit integers were treated as offsets into the document.
While the main reason of this PR is to fix the inline bug, we take this opportunity to:
Signed-off-by: Rohit Nayak rohit@planetscale.com
Error Snippet
A MoveTables failed due to the json parsing logic panicking for a binlog event with this error: