LE Audio shell strtoX fixes #54165

Thalley · 2023-01-27T13:02:23Z

Ensures that all strtoX calls are handled correctly, and moves to use the shell_strtoX functions

Fixes most of the bugs reported in #54101

finikorg · 2023-01-30T06:22:49Z

subsys/bluetooth/shell/vcp_vol_rend.c

-	int result;
-	int step = strtol(argv[1], NULL, 0);
+	unsigned long step;
+	int result = 0;


I think you should use the same name for error in all functions, err.

I disagree; I think it's better to just use the already defined value to keep the number of lines changed to a minimum.

finikorg · 2023-01-30T06:24:30Z

subsys/bluetooth/shell/vcp_vol_rend.c


-	if (step > UINT8_MAX || step == 0) {


looks like you changed logic, no check for step == 0 ?

Nice catch - Thanks!

finikorg · 2023-01-30T06:29:37Z

subsys/bluetooth/shell/vcp_vol_rend.c

+		return -ENOEXEC;
+	}
+
+	if (location > UINT32_MAX) {


the logic is changed, it was UINT16_MAX.

Yes, perhaps that should have been a separate commit. Should I add another commit for that?

Yes, I think

Added a commit to fix this in the PR

finikorg · 2023-01-30T06:38:18Z

subsys/bluetooth/shell/iso.c

-		count = strtoul(argv[1], NULL, 10);
+		count = shell_strtoul(argv[5], 0, &ret);
+		if (ret != 0) {
+			shell_error(sh, "Could not parse count: %d", ret);


better use the same variable for error, err.

finikorg · 2023-01-30T06:40:41Z

subsys/bluetooth/shell/media_controller.c

+	unsigned long order;
+	int err = 0;
+
+	order = shell_strtol(argv[1], 0, &err);


shell_strtoul ?

finikorg · 2023-01-30T13:46:47Z

subsys/bluetooth/shell/bap_broadcast_assistant.c


-		if (start_scan != 0 && start_scan != 1) {


shell_strtobool has different logic, at the end it converts unsigned long to bool, so any number > 0 is true, right?

Right. I was unsure about whether I should just use shell_strtobool directly, or actually check if the value was > 1, but decided on the former, as just using shell_strtobool would be more consistent with other shell commands that use that.

OK, looks reasonable.

finikorg

LGTM, although I would use the same error code err instead of result, etc.

Thalley · 2023-01-31T09:38:21Z

LGTM, although I would use the same error code err instead of result, etc.

Making all the audio shell implementations more consistent can be postponed to a separate PR :)

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Thalley · 2023-02-06T16:17:08Z

Found a few more with incorrect type

subsys/bluetooth/shell/mcc.c

kruithofa · 2023-02-06T14:03:33Z

subsys/bluetooth/shell/mcc.c

+	unsigned long order;
+	int result = 0;
+
+	order = shell_strtol(argv[1], 0, &result);


Suggested change

order = shell_strtol(argv[1], 0, &result);

order = shell_strtoul(argv[1], 0, &result);

subsys/bluetooth/shell/mcc.c

subsys/bluetooth/shell/tbs.c

kruithofa · 2023-02-07T10:36:55Z

subsys/bluetooth/shell/tbs_client.c

-			inst_index = strtol(argv[1], NULL, 0);
-			if (inst_index < 0 || inst_index > UINT8_MAX) {
-				shell_error(sh, "Invalid index");
+			inst_index = shell_strtoul(argv[1], 0, &result);


add a comment/TODO that this can be pulled out as a function since this code is repeated many times?

kruithofa · 2023-02-07T10:39:09Z

subsys/bluetooth/shell/vcp_vol_ctlr.c

 		return -ENOEXEC;
 	}

 	if (offset > BT_VOCS_MAX_OFFSET || offset < BT_VOCS_MIN_OFFSET) {
-		shell_error(sh, "Offset shall be %d-%d, was %d",
+		shell_error(sh, "Offset shall be %d-%d, was %ld",


missed opportunity for using IN_RANGE

kruithofa · 2023-02-07T10:40:59Z

subsys/bluetooth/shell/vcp_vol_ctlr.c

 	if (gain > INT8_MAX || gain < INT8_MIN) {
-		shell_error(sh, "Offset shall be %d-%d, was %d",
+		shell_error(sh, "Gain shall be %d-%d, was %d",


use IN_RANGE?

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

The location value is a uint32_t, and not uint16_t. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Thalley force-pushed the audio_shell_strtoX_fixes branch from 792398f to 7c5bb21 Compare January 27, 2023 14:25

Thalley mentioned this pull request Jan 27, 2023

bluetooth: shell: Lots of checks of type (unsigned < 0) which is bogus #54101

Closed

Thalley force-pushed the audio_shell_strtoX_fixes branch from 58ad87e to c8ff3b1 Compare January 29, 2023 12:10

Thalley added the bug The issue is a bug, or the PR is fixing a bug label Jan 29, 2023

Thalley added this to In progress in Bluetooth LE Audio via automation Jan 29, 2023

Thalley moved this from In progress to In Review in Bluetooth LE Audio Jan 29, 2023

Thalley marked this pull request as ready for review January 29, 2023 12:10

Thalley requested review from alwa-nordic, jhedberg and Vudentz as code owners January 29, 2023 12:10

Thalley requested review from cvinayak, kruithofa, MariuszSkamra, szymon-czapracki, asbjornsabo and Casper-Bonde-Bose January 29, 2023 12:11

zephyrbot added area: Bluetooth Audio area: Bluetooth labels Jan 29, 2023

zephyrbot requested review from fredrikdanebjer, hermabe, jori-nordic and sjanc January 29, 2023 12:11

zephyrbot assigned jhedberg Jan 29, 2023

finikorg requested changes Jan 30, 2023

View reviewed changes

Thalley force-pushed the audio_shell_strtoX_fixes branch from c8ff3b1 to a0fc36c Compare January 30, 2023 10:00

Thalley requested a review from finikorg January 30, 2023 12:19

finikorg requested changes Jan 30, 2023

View reviewed changes

Thalley requested a review from finikorg January 31, 2023 09:00

finikorg previously approved these changes Jan 31, 2023

View reviewed changes

Thalley added 6 commits February 6, 2023 17:11

Bluetooth: Audio: Ensure strtoX correctness in bap_broadcast_assistant.c

dcd10e8

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in bap_scan_delegator.c

eecbd0e

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in cap_acceptor.c

520007a

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in csip_set_coordinator.c

8255017

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in csip_set_member.c

be55fb3

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: ISO: Ensure strtoX correctness in iso.c

258c0c1

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Thalley dismissed MariuszSkamra’s stale review via 8869541 February 6, 2023 16:17

Thalley force-pushed the audio_shell_strtoX_fixes branch from 6c4c5df to 8869541 Compare February 6, 2023 16:17

Thalley requested a review from MariuszSkamra February 6, 2023 16:17

kruithofa requested changes Feb 7, 2023

View reviewed changes

Thalley added 10 commits February 7, 2023 11:52

Bluetooth: Audio: Ensure strtoX correctness in mcc.c

297d493

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in media_controller.c

6c16e12

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in micp_mic_ctlr.c

f0e4a80

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in micp_mic_dev.c

367d2b0

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in mpl.c

4bec442

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in tbs_client.c

ec5d244

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in tbs.c

de8023f

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Shell: Fix VCP location check

135b2ba

The location value is a uint32_t, and not uint16_t. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in vcp_vol_ctlr.c

be3eaac

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Bluetooth: Audio: Ensure strtoX correctness in vcp_vol_rend.c

59ab703

Verify all values returned by calls to strtoX and convert to the shell_strtoX functions. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

Thalley force-pushed the audio_shell_strtoX_fixes branch from 8869541 to 59ab703 Compare February 7, 2023 10:56

Thalley requested a review from kruithofa February 7, 2023 10:56

kruithofa approved these changes Feb 8, 2023

View reviewed changes

Thalley assigned Thalley and unassigned jhedberg Feb 13, 2023

carlescufi approved these changes Feb 20, 2023

View reviewed changes

carlescufi merged commit 9756aea into zephyrproject-rtos:main Feb 20, 2023

Bluetooth LE Audio automation moved this from In Review to Done Feb 20, 2023

Thalley deleted the audio_shell_strtoX_fixes branch March 8, 2023 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LE Audio shell strtoX fixes #54165

LE Audio shell strtoX fixes #54165

Thalley commented Jan 27, 2023 •

edited by laurenmurphyx64

finikorg Jan 30, 2023

Thalley Jan 30, 2023

finikorg Jan 30, 2023

Thalley Jan 30, 2023

finikorg Jan 30, 2023

Thalley Jan 30, 2023

finikorg Jan 30, 2023

Thalley Jan 30, 2023

finikorg Jan 30, 2023

finikorg Jan 30, 2023 •

edited

finikorg Jan 30, 2023

Thalley Jan 30, 2023

finikorg Jan 31, 2023

finikorg left a comment

Thalley commented Jan 31, 2023

Thalley commented Feb 6, 2023

kruithofa Feb 6, 2023

kruithofa Feb 7, 2023

kruithofa Feb 7, 2023

kruithofa Feb 7, 2023

	order = shell_strtol(argv[1], 0, &result);
	order = shell_strtoul(argv[1], 0, &result);

LE Audio shell strtoX fixes #54165

LE Audio shell strtoX fixes #54165

Conversation

Thalley commented Jan 27, 2023 • edited by laurenmurphyx64

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finikorg Jan 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finikorg left a comment

Choose a reason for hiding this comment

Thalley commented Jan 31, 2023

Thalley commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley commented Jan 27, 2023 •

edited by laurenmurphyx64

finikorg Jan 30, 2023 •

edited