shell/cord_ep: Add user guidance & prevent accidental crash #18053

Teufelchen1 · 2022-05-04T12:48:59Z

Contribution description

(Everything was done on the lastest master as of 2022-05-04T10:00)
This PR only touches the shell command handling of cord_ep
Not sure if I want to call it a fix; It fixes an issue where running the subcommand cord_ep register just slightly wrong, an assertion in gcoap.c would get triggered - resulting in a RIOT kernel panic.
I consider this behaviour to be a bug, reasoning that a shell should be able to cope with users that do not exactly know what they are doing. (Go figure who that user was 😇)
This PR corrects this behaviour by doing a short sanity check on the user input - yielding an error if necessary.

Testing procedure

For reproducing the bug do:

Go to RIOT/examples/cord_ep/ and run make all flash term for a board of your choice
Inside the RIOT shell, run cord_ep register [fd00::1]:1234 foo (the exact host:port combination does not matter, just have to be valid)
Make sure that the registration interface (in my example foo) does not start with a /
Observe a thrown failed assertion i.g., *** RIOT kernel panic: FAILED ASSERTION.

After applying my suggested fix:

Go to RIOT/examples/cord_ep/ and run make all flash term for a board of your choice
Inside the RIOT shell, run cord_ep register [fd00::1]:1234 foo (the exact host:port combination does not matter, just have to be valid)
Make sure that the registration interface (in my example foo) does not start with a /
The command responds with an error: error: registration interface must start with '/'

leandrolanzieri · 2022-05-05T11:41:55Z

sys/shell/commands/sc_cord_ep.c

@@ -63,6 +63,10 @@ int _cord_ep_handler(int argc, char **argv)
        }
        if (argc > 3) {
            regif = argv[3];
+            if (regif[0] != '/') {


Perhaps it makes sense just to change the command and accept an URI, which we parse with https://doc.riot-os.org/group__sys__uri__parser.html ?

Hm. A reasonable suggestion but should the shell command enforce that the registration interface is a valid uri?
At the moment the chain of responsibility is:

registration interface -> defined somewhere in some coap related rfc

gcoap_req_init() -> uses a registration interface

gcoap_req_init() -> enforces some bare minimal expectation how a registration interface should look like (mostly for programming safety I assume, didn't look deeply at it)

cord_ep_register() -> passes a registration interface(without any checks on it, beside a convenience NULL handling) to gcoap_req_init()

_cord_ep_handler() -> calls cord_ep_register() with a user given registration interface (without any checks on it)

the user -> "Generates" a registration interface and calls _cord_ep_handler() with it (maybe with checks, maybe without, users tend to be unreliable 😬)

Given this chain & your proposal, I think I could look up in coap / the rfc how exactly a registration interface may and or must look like (URI...). After that is clarified (at least for me :p) one should enforce that as close to the rfc as possible (in the chain). One aspect that is important for deciding this is performance / code size etc. E.g. a heavy URI check may not be wanted in 3. gcoap, assuming that only other software interacts with it(but not users). This would delegate that responsibility to 4. cord_ep_register().
Enforcing the registration interface to be compliant with a given uri parser inside the shell command, while the e.g. cord_ep_register() could perfectly work with a non uri compliant registration interface, could become confusing.

E.g. A user could find that a given registration interface works in his/her own code but not in the shell. This is also the reason why I tried to match the assert as close as possible and didn't add any other checks.

Given this chain & your proposal, I think I could look up in coap / the rfc how exactly a registration interface may and or must look like (URI...). After that is clarified (at least for me :p) one should enforce that as close to the rfc as possible (in the chain). One aspect that is important for deciding this is performance / code size etc. E.g. a heavy URI check may not be wanted in 3. gcoap, assuming that only other software interacts with it(but not users). This would delegate that responsibility to 4. cord_ep_register(). Enforcing the registration interface to be compliant with a given uri parser inside the shell command, while the e.g. cord_ep_register() could perfectly work with a non uri compliant registration interface, could become confusing.

E.g. A user could find that a given registration interface works in his/her own code but not in the shell. This is also the reason why I tried to match the assert as close as possible and didn't add any other checks.

As I understand this will always end up being a valid URI. We need to get from the command the RD address (maybe port) pointing to a CoAP endpoint, and CoAP path (i.e. the interface). I think unifying the shell commands through our examples is a big gain, then we don't re-implement parsers each time. My proposal is to merge the argument 2 and 3 (remote and interface) into a single URI.

chrysn · 2022-09-22T06:59:07Z

The RFC is of little help here -- it only deals in URIs (and not URI references); the argument to cord_ep register is neither. (We could make it a limited URI reference, but #18622 shows it's not there now).

Going through a full URI here is one solution, but then it should be applied consistently both here and to the coap commands of the gcoap example (which, by the way, suffer the same bug). The current PR does that, and it's looking good at first glance.

(I think we should also do that for the gcoap example. Doesn't need to be, probably even shouldn't be, in this PR -- setting an example here puts a bit of pressure on the other example).

One important distinction is getting a bit lost here, though: With an RD, you can either start with a host (and then perform autodiscovery) or with a resource (like, the user did the discovery, here's the URI to go to). That resource could even be / (it's a corner case but definitely possible). With the current interface, that distinction is not visible any more. I think that the distinction is implicit here because uri_result.path might be NULL and thus trigger auto-discovery, and that's something the user can work with (esp. if it's communicated well); I'm giving it a try.

By the way, is this still WIP?

chrysn

Looks good; see above for nits and comments.

If you remove the WIP and declare it done from your side, I think this can go in.

sys/shell/cmds/cord_ep.c

chrysn · 2022-09-22T07:30:24Z

sys/shell/cmds/cord_ep.c

+        if (uri_result.ipv6addr == NULL) {
+            puts("error: Only ipv6 addresses are supported");
+            return 1;
+        }
+
        sock_udp_ep_t remote;
-        if (make_sock_ep(&remote, argv[2]) < 0) {
-            printf("error: unable to parse address\n");
+        remote.family = AF_INET6;
+        remote.netif = SOCK_ADDR_ANY_NETIF;
+        if (!ipv6_addr_from_buf((ipv6_addr_t *)&remote.addr.ipv6, uri_result.ipv6addr,
+                                uri_result.ipv6addr_len)) {
+            puts("error: IPv6 address malformed");
            return 1;
        }
-        if (argc > 3) {
-            regif = argv[3];
+
+        if (uri_result.port == 0) {
+            remote.port = COAP_PORT;
        }
+        else {
+            remote.port = uri_result.port;
+        }


This could go into a helper function (possibly part of the URI parsing module) that extracts a sock_udp_ep_t from a parsed URI (being provided a default port).

(Then, also, that'd be a unified place where handling DNS names could be done, or legacy IP versions could be handled if anyone cared).

Pssssst 🤫 you are leaking my secret plans for the uri parser.

I'm going to move the uri_parser discussion in separate issue. :)

Fine with me; all the more reason to get this done, so you can follow up, and #18622 will become easy :-)

sys/shell/cmds/cord_ep.c

Teufelchen1 · 2022-09-22T10:02:20Z

The RFC is of little help here -- it only deals in URIs (and not URI references); the argument to cord_ep register is neither. (We could make it a limited URI reference, but #18622 shows it's not there now).

What ever we do, I strongly suggest to unify the ux/ui - as already pointed out by Leandro. "I think unifying the shell commands through our examples is a big gain, then we don't re-implement parsers each time."

Going through a full URI here is one solution,
[...]
-- setting an example here puts a bit of pressure on the other example).

😈

One important distinction is getting a bit lost here, though: With an RD, you can either start with a host (and then perform autodiscovery) or with a resource (like, the user did the discovery, here's the URI to go to). That resource could even be / (it's a corner case but definitely possible). With the current interface, that distinction is not visible any more. I think that the distinction is implicit here because uri_result.path might be NULL and thus trigger auto-discovery, and that's something the user can work with (esp. if it's communicated well); I'm giving it a try.

Don't forget that cord_ep discover <address> still exist! If you actively want to discover the resource, you still can do so and it is not a hidden feature. Additionally, resorting to auto discovery when using register without providing a resource is a good ux for a shell command. It is a bit of a pity that / is no longer distinguishable but imo acceptable for a shell command. Given my own experience, shell commands are the first things a newcomer is going to play with in riot, they should be simple, helpful and friendly. "Professional users" writing "production software" aren't likely to use shell commands in production - those are not affected by this / edge-case.

By the way, is this still WIP?

Yes! Since this file is now outdated. I would have fixed it yesterday but sadly this part of riot is either completely undocumented or I couldn't find documentation about it. This caused me to waste so much time on getting this "test" to run & understand that I lost motivation and called it a day....

chrysn · 2022-09-22T10:51:20Z

Don't forget that cord_ep discover <address> still exist!

I did indeed miss it. It's good to have it around so users are able to introspect what's happening -- and if I understand your sentiment right, things will still Just Work especially for users who don't provide a path and get auto-discovery.

I would have fixed it yesterday but sadly this part of riot is either completely undocumented or I couldn't find documentation about it.

I feel this, riotctl is a cluster of things (configuration? building? CLI abstraction?) that I don't get myself either, and only fix superficially whenever something breaks. Before you get stuck, don't hesitate to ping is authors for help. Tests should keep software good, but not at the cost of keeping it from getting better.

sys/shell/cmds/cord_ep.c

chrysn · 2022-09-22T12:32:28Z

I don't think that RD registration via CoAPS was ever tested with this implementation. (And how would it have been set -- it never took an indictor). Just working for `coap` initially is probably a good start. Once not only the set-the-socket-from-the-parsed-URI but also its caller, prepare-the-request-from-the-parsed-URI (which passes in the default port) are factored out, we can still think of those.

chrysn · 2022-09-22T16:57:44Z

One more request before I ACK this: Please ensure the commit message is current; I suggest something like "shell/cord_ep: Take full URIs instead of IP and path" / / "This fixes a crash when the path was not entered with a leading slash."

[note to self: And then there's the case of riotctrl_shell/tests that I have no clue how to go about other than pinging people on Matrix]

chrysn · 2022-09-23T12:24:19Z

Having spent some time with the riotctrl_shell/cord_ep, I don't think that file needs changes. Given that it does nothing to abstract, and where it does (by naming arguments) it does wrongly (it says "uri", which is correct now but wasn't correct before), it can stay as it is, and its callers will need to adjust. As the release tests are maintained out-of-tree, this will only happen after this PR is in. (It may also happen to be practical to just move this test in-tree).

This fixes a crash when the path was not entered with a leading slash.

Teufelchen1 · 2022-10-06T13:34:07Z

Ping @chrysn -
Murdock is stuck at "Expected — Waiting for status to be reported" what ever that means. Beside that, hit merge? 🥺❤️

chrysn · 2022-10-06T20:37:59Z

Thanks for fixing this!

The argument regif was removed from the shell command in RIOT-OS#18053, so there is not much need to keep it in the ShellInteraction for that command.

github-actions bot added the Area: sys Area: System label May 4, 2022

leandrolanzieri reviewed May 5, 2022

View reviewed changes

benpicco requested a review from chrysn May 8, 2022 20:48

Teufelchen1 changed the title ~~shell/cord_ep: Add user guidance & prevent accidental crash~~ WIP: shell/cord_ep: Add user guidance & prevent accidental crash May 11, 2022

Teufelchen1 mentioned this pull request May 12, 2022

sys/uri_parser: Adding the port as uint16_t #18096

Merged

Teufelchen1 force-pushed the fix/cord_ep branch from d34dcdb to 6a11a81 Compare September 21, 2022 15:54

chrysn reviewed Sep 22, 2022

View reviewed changes

chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CoAP Area: Constrained Application Protocol implementations and removed Area: sys Area: System labels Sep 22, 2022

Teufelchen1 force-pushed the fix/cord_ep branch from 6a11a81 to d73ef97 Compare September 22, 2022 10:14

github-actions bot added Area: sys Area: System and removed Area: CoAP Area: Constrained Application Protocol implementations labels Sep 22, 2022

chrysn mentioned this pull request Sep 22, 2022

Command line CoAP client does not support queries #18622

Open

Teufelchen1 force-pushed the fix/cord_ep branch from d73ef97 to b83dce0 Compare September 22, 2022 12:10

Teufelchen1 commented Sep 22, 2022

View reviewed changes

sys/shell/cmds/cord_ep.c Outdated Show resolved Hide resolved

Teufelchen1 force-pushed the fix/cord_ep branch from b83dce0 to 14087a0 Compare September 22, 2022 16:43

chrysn mentioned this pull request Sep 23, 2022

examples/cord_ep: Move release test 09-coap #01 into RIOT tree #18633

Open

shell/cord_ep: Take full URIs instead of IP and path

3dc3110

This fixes a crash when the path was not entered with a leading slash.

Teufelchen1 force-pushed the fix/cord_ep branch from 14087a0 to 3dc3110 Compare September 27, 2022 14:31

Teufelchen1 changed the title ~~WIP: shell/cord_ep: Add user guidance & prevent accidental crash~~ shell/cord_ep: Add user guidance & prevent accidental crash Sep 27, 2022

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 27, 2022

kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 27, 2022

Teufelchen1 mentioned this pull request Oct 6, 2022

Future roadmap of the uri_parser? #18702

Open

chrysn approved these changes Oct 6, 2022

View reviewed changes

chrysn merged commit edfa255 into RIOT-OS:master Oct 6, 2022

miri64 mentioned this pull request Oct 8, 2022

release-tests: use Ubuntu Jammy runner #18712

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2022

riotctrl_shell.cord_ep: remove regif parameter

fa72453

The argument regif was removed from the shell command in RIOT-OS#18053, so there is not much need to keep it in the ShellInteraction for that command.

This was referenced Oct 10, 2022

09-coap: use URI with cord_ep register RIOT-OS/Release-Specs#260

Merged

riotctrl_shell.cord_ep: remove regif parameter #18716

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2022

riotctrl_shell.cord_ep: remove regif parameter

8c04964

The argument regif was removed from the shell command in RIOT-OS#18053, so there is not much need to keep it in the ShellInteraction for that command.

maribu added this to the Release 2022.10 milestone Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shell/cord_ep: Add user guidance & prevent accidental crash #18053

shell/cord_ep: Add user guidance & prevent accidental crash #18053

Teufelchen1 commented May 4, 2022

leandrolanzieri May 5, 2022

Teufelchen1 May 5, 2022

leandrolanzieri May 5, 2022

chrysn commented Sep 22, 2022

chrysn left a comment

chrysn Sep 22, 2022

Teufelchen1 Sep 22, 2022

chrysn Sep 22, 2022

Teufelchen1 commented Sep 22, 2022

chrysn commented Sep 22, 2022

chrysn commented Sep 22, 2022 via email

chrysn commented Sep 22, 2022 •

edited

chrysn commented Sep 23, 2022

Teufelchen1 commented Oct 6, 2022

chrysn commented Oct 6, 2022

shell/cord_ep: Add user guidance & prevent accidental crash #18053

shell/cord_ep: Add user guidance & prevent accidental crash #18053

Conversation

Teufelchen1 commented May 4, 2022

Contribution description

Testing procedure

leandrolanzieri May 5, 2022

Choose a reason for hiding this comment

Teufelchen1 May 5, 2022

Choose a reason for hiding this comment

leandrolanzieri May 5, 2022

Choose a reason for hiding this comment

chrysn commented Sep 22, 2022

chrysn left a comment

Choose a reason for hiding this comment

chrysn Sep 22, 2022

Choose a reason for hiding this comment

Teufelchen1 Sep 22, 2022

Choose a reason for hiding this comment

chrysn Sep 22, 2022

Choose a reason for hiding this comment

Teufelchen1 commented Sep 22, 2022

chrysn commented Sep 22, 2022

chrysn commented Sep 22, 2022 via email

chrysn commented Sep 22, 2022 • edited

chrysn commented Sep 23, 2022

Teufelchen1 commented Oct 6, 2022

chrysn commented Oct 6, 2022

chrysn commented Sep 22, 2022 •

edited