Skip to content

Commit

Permalink
Fix parser bug around MSG protocol.
Browse files Browse the repository at this point in the history
Make sure we do the right thing when no args are presented for an MSG, e.g. MSG <spc>.
Also do not parse at all of this is a client, only valid for routes.
  • Loading branch information
derekcollison committed Aug 27, 2015
1 parent a0ad3c7 commit 8fb92dc
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 7 deletions.
8 changes: 5 additions & 3 deletions server/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,9 +374,6 @@ func (c *client) processMsgArgs(arg []byte) error {
args = append(args, arg[start:])
}

c.pa.subject = args[0]
c.pa.sid = args[1]

switch len(args) {
case 3:
c.pa.reply = nil
Expand All @@ -392,6 +389,11 @@ func (c *client) processMsgArgs(arg []byte) error {
if c.pa.size < 0 {
return fmt.Errorf("processMsgArgs Bad or Missing Size: '%s'", arg)
}

// Common ones processed after check for arg length
c.pa.subject = args[0]
c.pa.sid = args[1]

return nil
}

Expand Down
8 changes: 6 additions & 2 deletions server/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ func (c *client) parse(buf []byte) error {
case 'U', 'u':
c.state = OP_U
case 'M', 'm':
c.state = OP_M
if c.typ == CLIENT {
goto parseErr
} else {

This comment has been minimized.

Copy link
@kozlovic

kozlovic Aug 27, 2015

Member

nit: do we need else since we have goto in above "if"... just try to learn coding best practices...

This comment has been minimized.

Copy link
@derekcollison

derekcollison Aug 27, 2015

Author Member

Technically no, but I like being explicit myself.

c.state = OP_M
}
case 'C', 'c':
c.state = OP_C
case 'I', 'i':
Expand Down Expand Up @@ -613,7 +617,7 @@ func (c *client) parse(buf []byte) error {
c.state == CONNECT_ARG) && c.argBuf == nil {
c.argBuf = c.scratch[:0]
c.argBuf = append(c.argBuf, buf[c.as:i-c.drop]...)
// FIXME, check max len
// FIXME(dlc), check max control line len
}
// Check for split msg
if (c.state == MSG_PAYLOAD || c.state == MSG_END) && c.msgBuf == nil {
Expand Down
22 changes: 21 additions & 1 deletion server/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ func dummyClient() *client {
return &client{}
}

func dummyRouteClient() *client {
return &client{typ: ROUTER}
}

func TestParsePing(t *testing.T) {
c := dummyClient()
if c.state != OP_START {
Expand Down Expand Up @@ -233,7 +237,7 @@ func TestParsePubBadSize(t *testing.T) {
}

func TestParseMsg(t *testing.T) {
c := dummyClient()
c := dummyRouteClient()

pub := []byte("MSG foo RSID:1:2 5\r\nhello\r")
err := c.parse(pub)
Expand Down Expand Up @@ -310,6 +314,22 @@ func TestParseMsgArg(t *testing.T) {
testMsgArg(c, t)
}

func TestParseMsgSpace(t *testing.T) {
c := dummyRouteClient()

// Ivan bug he found
if err := c.parse([]byte("MSG \r\n")); err == nil {
t.Fatalf("Expected parse error for MSG <SPC>")
}

c = dummyClient()

// Anything with an M from a client should parse error
if err := c.parse([]byte("M")); err == nil {
t.Fatalf("Expected parse error for M* from a client")
}
}

func TestShouldFail(t *testing.T) {
c := dummyClient()

Expand Down
4 changes: 3 additions & 1 deletion server/split_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,8 @@ func TestSplitDanglingArgBuf(t *testing.T) {

func TestSplitMsgArg(t *testing.T) {
_, c, _ := setupClient()
// Allow parser to process MSG
c.typ = ROUTER

b := make([]byte, 1024)

Expand Down Expand Up @@ -371,7 +373,7 @@ func TestSplitMsgArg(t *testing.T) {
}

func TestSplitBufferMsgOp(t *testing.T) {
c := &client{subs: hashmap.New()}
c := &client{subs: hashmap.New(), typ: ROUTER}
msg := []byte("MSG foo.bar QRSID:15:3 _INBOX.22 11\r\nhello world\r")
msg1 := msg[:2]
msg2 := msg[2:9]
Expand Down

0 comments on commit 8fb92dc

Please sign in to comment.