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

examples/observe/server: fix buf usage #329

Merged
merged 1 commit into from
May 11, 2022
Merged

examples/observe/server: fix buf usage #329

merged 1 commit into from
May 11, 2022

Conversation

maribu
Copy link
Contributor

@maribu maribu commented May 11, 2022

The observe server example uses the same buffer for both the content of
the Content-Format and the Observe option. This does work int he example
by accident, as the used Content-Format is 0 and, hence, is encoded with
a zero-length option value. If modified like this, the issue becomes
apparent:

diff --git a/examples/observe/server/main.go b/examples/observe/server/main.go
index 08664a6..6ab6119 100644
--- a/examples/observe/server/main.go
+++ b/examples/observe/server/main.go
@@ -30,10 +30,10 @@ func sendResponse(cc mux.Client, token []byte, subded time.Time, obs int64) erro
     }
     var opts message.Options
     var buf []byte
-    opts, n, err := opts.SetContentFormat(buf, message.TextPlain)
+    opts, n, err := opts.SetContentFormat(buf, message.AppJSON)
     if err == message.ErrTooSmall {
         buf = append(buf, make([]byte, n)...)
-        opts, _, err = opts.SetContentFormat(buf, message.TextPlain)
+        opts, _, err = opts.SetContentFormat(buf, message.AppJSON)
     }
     if err != nil {
         return fmt.Errorf("cannot set content format to response: %w", err)

As message.AppJSON is a non-zero value, it will actually write content
to the buffer buf. This buffer is subsequently reused for the Observe
option and the value it holds (the 50 for AppJSON) is overwritten by
the Observe Option content, corrupting the outgoing message. (This can
easily verified with Wireshark, which shows the content format to count
up with the observe number after applying the patch above).

This commit uses different buffers for each option. Note that while this
bug is not triggered with message.TextPlain, application developers will
base their code on this example and are bound to be hitting this bug the
moment the use a different content format.

}
if err != nil {
return fmt.Errorf("cannot set content format to response: %w", err)
}
var obsbuf []byte
Copy link
Member

Choose a reason for hiding this comment

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

Using two buffers works, though i'd say the idiomatic way is to update the buffer head to be past the bytes written by the last option:

var opts message.Options
var buf []byte
opts, n, err := opts.SetContentFormat(buf, message.TextPlain)
if err == message.ErrTooSmall {
	buf = append(buf, make([]byte, n)...)
	opts, n, err = opts.SetContentFormat(buf, message.TextPlain)
}
if err != nil {
	return fmt.Errorf("cannot set content format to response: %w", err)
}

buf = buf[n:]

if obs >= 0 {
	opts, n, err = opts.SetObserve(buf, uint32(obs))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as suggested. Shall I squash?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, squash and you can merge, good job!

The observe server example uses the same buffer for both the content of
the Content-Format and the Observe option. This does work int he example
by accident, as the used Content-Format is 0 and, hence, is encoded with
a zero-length option value. If modified like this, the issue becomes
apparent:

```diff
diff --git a/examples/observe/server/main.go b/examples/observe/server/main.go
index 08664a6..6ab6119 100644
--- a/examples/observe/server/main.go
+++ b/examples/observe/server/main.go
@@ -30,10 +30,10 @@ func sendResponse(cc mux.Client, token []byte, subded time.Time, obs int64) erro
     }
     var opts message.Options
     var buf []byte
-    opts, n, err := opts.SetContentFormat(buf, message.TextPlain)
+    opts, n, err := opts.SetContentFormat(buf, message.AppJSON)
     if err == message.ErrTooSmall {
         buf = append(buf, make([]byte, n)...)
-        opts, _, err = opts.SetContentFormat(buf, message.TextPlain)
+        opts, _, err = opts.SetContentFormat(buf, message.AppJSON)
     }
     if err != nil {
         return fmt.Errorf("cannot set content format to response: %w", err)
```

As message.AppJSON is a non-zero value, it will actually write content
to the buffer buf. This buffer is subsequently reused for the Observe
option and the value it holds (the 50 for AppJSON) is overwritten by
the Observe Option content, corrupting the outgoing message. (This can
easily verified with Wireshark, which shows the content format to count
up with the observe number after applying the patch above).

This commit uses different buffers for each option. Note that while this
bug is not triggered with message.TextPlain, application developers will
base their code on this example and are bound to be hitting this bug the
moment the use a different content format.
@jkralik jkralik merged commit b9a97af into plgd-dev:master May 11, 2022
@jkralik
Copy link
Member

jkralik commented May 11, 2022

Thx for your contribution!

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.

None yet

3 participants