Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

use varints for delimiting plaintext 2.0 msgs #74

Merged
merged 3 commits into from
Nov 12, 2019

Conversation

yusefnapora
Copy link
Contributor

This fixes a bug in plaintext 2.0, which according to the spec should use varints for delimiting messages. I used go-msgio thinking that it used varints under the hood, but it actually uses 4 byte unsigned ints instead.

This switches out msgio for gogoprotobuf's varint delimited Reader / Writer types.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't think we need tests since this was a one-off, and any test we write to cover this would actually be testing gogo's ability to length-encode.

sec/insecure/insecure.go Outdated Show resolved Hide resolved
sec/insecure/insecure.go Outdated Show resolved Hide resolved
Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I don't know Go 🤷‍♂️

@yusefnapora yusefnapora merged commit f06e38f into master Nov 12, 2019
@yusefnapora yusefnapora deleted the fix/plaintext-varint branch November 12, 2019 16:53
@mxinden
Copy link
Member

mxinden commented Nov 12, 2019

Thanks for fixing this so quickly!

func readWriteMsg(rw io.ReadWriter, out *pb.Exchange) (*pb.Exchange, error) {
const maxMsgSize = 1 << 16
r := gogoio.NewDelimitedReader(rw, maxMsgSize)
w := gogoio.NewDelimitedWriter(rw)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will over-read and corrupt the stream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it buffers internally)

@Stebalien Stebalien restored the fix/plaintext-varint branch April 3, 2020 03:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants