Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spkt: check for minimum common header length #1987

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

sgmonroy
Copy link
Contributor

@sgmonroy sgmonroy commented Oct 16, 2018

This change is Reviewable

Copy link
Contributor

@kormat kormat left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sgmonroy)


go/lib/spkt/cmnhdr.go, line 56 at r1 (raw file):

	// Check for minimum header length
	if len(b) < CmnHdrLen {
		return common.NewBasicError("Packet is too small", nil, "Size", len(b))

I'd be a bit more specific, and also not assume this is from a packet: "Can't parse common header, buffer is too short"

We also generally use lower-case for the logging keys ("size" rather than "size"), and use len rather than size.

Copy link
Contributor Author

@sgmonroy sgmonroy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @kormat)


go/lib/spkt/cmnhdr.go, line 56 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'd be a bit more specific, and also not assume this is from a packet: "Can't parse common header, buffer is too short"

We also generally use lower-case for the logging keys ("size" rather than "size"), and use len rather than size.

Done.

Copy link
Contributor

@kormat kormat left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@sgmonroy sgmonroy merged commit b44c076 into scionproto:master Oct 16, 2018
@sgmonroy sgmonroy deleted the br-fix-small-packet branch October 18, 2018 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants