code review 14438068: go.crypto/ssh: (un)marshal data without type byte prefix. (issue 14438068)

[han...@google.com]

Reviewers: agl1, dfc, jpsugar,

Message:
Hello agl1, dfc, jps...@google.com (cc: golan...@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto


Description:
go.crypto/ssh: (un)marshal data without type byte prefix.

This helps manipulating data in global and channel request
payloads.

Please review this at https://codereview.appspot.com/14438068/

Affected files (+49, -8 lines):
M ssh/messages.go
M ssh/messages_test.go


Index: ssh/messages.go
===================================================================
--- a/ssh/messages.go
+++ b/ssh/messages.go
@@ -227,17 +227,20 @@
PubKey string
}

-// unmarshal parses the SSH wire data in packet into out using reflection.
-// expectedType is the expected SSH message type. It either returns nil on
+// unmarshal parses the SSH wire data in packet into out using
+// reflection. expectedType, if positive, is the expected SSH message
+// type that the packet should start with. It either returns nil on
// success, or a ParseError or UnexpectedMessageError on error.
func unmarshal(out interface{}, packet []byte, expectedType uint8) error {
if len(packet) == 0 {
return ParseError{expectedType}
}
- if packet[0] != expectedType {
- return UnexpectedMessageError{expectedType, packet[0]}
+ if expectedType > 0 {
+ if packet[0] != expectedType {
+ return UnexpectedMessageError{expectedType, packet[0]}
+ }
+ packet = packet[1:]
}
- packet = packet[1:]

v := reflect.ValueOf(out).Elem()
structType := v.Type()
@@ -319,10 +322,16 @@
return nil
}

-// marshal serializes the message in msg, using the given message type.
+// marshal serializes the message in msg. The given message type is
+// prepended if it is positive.
func marshal(msgType uint8, msg interface{}) []byte {
- out := make([]byte, 1, 64)
- out[0] = msgType
+ var out []byte
+ if msgType > 0 {
+ out = make([]byte, 1, 64)
+ out[0] = msgType
+ } else {
+ out = make([]byte, 0, 64)
+ }

v := reflect.ValueOf(msg)
for i, n := 0, v.NumField(); i < n; i++ {
Index: ssh/messages_test.go
===================================================================
--- a/ssh/messages_test.go
+++ b/ssh/messages_test.go
@@ -78,6 +78,38 @@
}
}

+func TestBareMarshalUnmarshal(t *testing.T) {
+ type S struct {
+ I uint32
+ S string
+ B bool
+ }
+
+ s := S{42, "hello", true}
+ packet := marshal(0, s)
+ roundtrip := S{}
+ unmarshal(&roundtrip, packet, 0)
+
+ if !reflect.DeepEqual(s, roundtrip) {
+ t.Errorf("got %#v, want %#v", roundtrip, s)
+ }
+}
+
+func TestBareMarshal(t *testing.T) {
+ type S2 struct {
+ I uint32
+ }
+ s := S2{42}
+ packet := marshal(0, s)
+ i, rest, ok := parseUint32(packet)
+ if len(rest) > 0 || !ok {
+ t.Errorf("parseInt(%q): parse error", packet)
+ }
+ if i != s.I {
+ t.Errorf("got %d, want %d", i, s.I)
+ }
+}
+
func randomBytes(out []byte, rand *rand.Rand) {
for i := 0; i < len(out); i++ {
out[i] = byte(rand.Int31())

[jps...@google.com]

LGTM

https://codereview.appspot.com/14438068/

[a...@golang.org]

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go
File ssh/messages.go (right):

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go#newcode231
ssh/messages.go:231: // reflection. expectedType, if positive, is the
expected SSH message
expectedType is unsigned, so always positive. Do you mean "if non-zero"?

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go#newcode231
ssh/messages.go:231: // reflection. expectedType, if positive, is the
expected SSH message
single space after period.

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go#newcode326
ssh/messages.go:326: // prepended if it is positive.
s/positive/non-zero.

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go#newcode329
ssh/messages.go:329: if msgType > 0 {

out := make([]byte, 0, 64)

if msgType > 0 {
out = append(out, msgType)
}

https://codereview.appspot.com/14438068/

[han...@google.com]

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go
File ssh/messages.go (right):

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go#newcode231
ssh/messages.go:231: // reflection. expectedType, if positive, is the
expected SSH message

On 2013/10/14 20:52:20, agl1 wrote:
> single space after period.

Done.

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go#newcode231
ssh/messages.go:231: // reflection. expectedType, if positive, is the
expected SSH message

On 2013/10/14 20:52:20, agl1 wrote:
> expectedType is unsigned, so always positive. Do you mean "if
non-zero"?

positive numbers are non-zero by definition? Agree that non-zero is
clearer, though.

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go#newcode326
ssh/messages.go:326: // prepended if it is positive.

On 2013/10/14 20:52:20, agl1 wrote:
> s/positive/non-zero.

Done.

https://codereview.appspot.com/14438068/diff/7001/ssh/messages.go#newcode329
ssh/messages.go:329: if msgType > 0 {

On 2013/10/14 20:52:20, agl1 wrote:
> out := make([]byte, 0, 64)
> if msgType > 0 {
> out = append(out, msgType)
> }

Done.

https://codereview.appspot.com/14438068/

[da...@cheney.net]

LGTM.


https://codereview.appspot.com/14438068/diff/11001/ssh/messages.go
File ssh/messages.go (right):

https://codereview.appspot.com/14438068/diff/11001/ssh/messages.go#newcode232
ssh/messages.go:232: // type that the packet should start with. It
either returns nil on
s/It/unmarshal

https://codereview.appspot.com/14438068/

[han...@google.com]

https://codereview.appspot.com/14438068/diff/11001/ssh/messages.go
File ssh/messages.go (right):

https://codereview.appspot.com/14438068/diff/11001/ssh/messages.go#newcode232
ssh/messages.go:232: // type that the packet should start with. It
either returns nil on

On 2013/10/15 04:00:01, dfc wrote:
> s/It/unmarshal

Done.

https://codereview.appspot.com/14438068/

[Dave Cheney]

Thanks, submitting.

> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/golang-dev/047d7b15afd580fe0804e8c0aaed%40google.com.
>
> For more options, visit https://groups.google.com/groups/opt_out.

[da...@cheney.net]

*** Submitted as
https://code.google.com/p/go/source/detail?r=ef3d7138b0b5&repo=crypto
***

go.crypto/ssh: (un)marshal data without type byte prefix.

This helps manipulating data in global and channel request
payloads.

R=agl, dave, jpsugar
CC=golang-dev
https://codereview.appspot.com/14438068

Committer: Dave Cheney <da...@cheney.net>


https://codereview.appspot.com/14438068/

[han...@google.com]

*** Abandoned ***

https://codereview.appspot.com/14438068/