snapshot: read meta.json correctly. #5193
Merged
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
banks
approved these changes
Jan 7, 2019
| @@ -193,8 +194,11 @@ func read(in io.Reader, metadata *raft.SnapshotMeta, snap io.Writer) error { | |||
|
|
|||
| switch hdr.Name { | |||
| case "meta.json": | |||
| dec := json.NewDecoder(io.TeeReader(archive, metaHash)) | |||
| if err := dec.Decode(&metadata); err != nil { | |||
| buf, err := ioutil.ReadAll(io.TeeReader(archive, metaHash)) | |||
There was a problem hiding this comment.
I think it's worth commenting on why this is needed. The root cause seems to be something like:
// json.Decode will stop reading as soon as the initial JSON element (the object) is closed
// (i.e. at the closing brace) which means the final newline, if any, is not read and so is not
// delivered to `metaHash`. This means that the hash calculated is not the same as the
// one in SHA256SUMS which was calculated over the raw bytes in the file. By explicitly
// reading the whole thing first we ensure that we calculate the hash over the exact
// same bytes regardless of whitespace.
There was a problem hiding this comment.
Good point, I will add a comment!
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.
Fixes #4452.
I basically did what @maf23 suggested, and made sure we read the file entirely. Since we don't need the json decoder in this case I read the file first and unmarshal afterwards. Apparently
ReadAllreads everything in contrast toDecode. I added a test with the file provided by @maf23 which couldn't be verified before even though it is a valid snapshot. This will prevent us from changing the decoding in a way that runs into that same problem again.I do think this is a very strange bug btw.