net/nanocoap: fix string option separator write handling #10223
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kb2ma
added
Type: bug
haukepetersen
requested changes
Nov 21, 2018
haukepetersen
added
Reviewed: 1-fundamentals
haukepetersen
approved these changes
Nov 22, 2018
Assumed initial character was a separator when writing the option, and skipped over it.
kb2ma
force-pushed
the
nanocoap/string_opt_first_char
branch
from
081b640 to
fa77929
Compare
kb2ma
added
the
CI: ready for build
|
Yay, our automated overlords have been appeased! I will rebase #9156 after the merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
Both the nanocoap
coap_opt_put_string()andcoap_opt_add_string()functions assume the first character in the string is a separator, and skip over it when writing the (first) option. This behavior likely resulted from our initial handling of Path strings. For a path we usually send the full concatenated path as a single string, which always starts with a '/', at least for a Uri-Path option.However, for a Uri-Query we usually send each query individually, for example in the
cord_common_add_qstring()function, and we don't expect to start the query key with an '&'.The error is not expressed currently in the cord module because gcoap uses an older mechanism to write a query option. I'm trying to update how gcoap writes options in some other work, and encountered the problem.
Testing procedure
Probably the easiest way to test is to copy/paste the included unit test, test_nanocoap__get_query() into the tests-nanocoap unit test in master. It tries to add the query string 'ab=cde'. You'll see the test fails because the length of the option is short by one. It neglects to write the first character in the key, so the option contains 'b=cde'.