drivers: sensors: sensor_shell: fix hang & crash #72815

ycsin · 2024-05-15T16:31:33Z

Currently, getting any channel >= SENSOR_CHAN_VSHUNT would result in a NULL pointer deref:

uart:~$ sensor get sensor@0 32
channel idx=32 (null) shift=6 num_samples=1 value=9220133425350000000ns (32.000000)
uart:~$ sensor get sensor@0 current
[870:26:48.134,000] <err> os: 
[870:26:48.134,000] <err> os:  mcause: 5, Load access fault
[870:26:48.134,000] <err> os:   mtval: 0
[870:26:48.134,000] <err> os:      a0: 0000000080016b36    t0: 000000008000dfc0
[870:26:48.134,000] <err> os:      a1: 0000000000000000    t1: 0000000000000039
[870:26:48.134,000] <err> os:      a2: 0000000000000063    t2: 0000000000000000
[870:26:48.134,000] <err> os:      a3: 0000000000000076    t3: 000000000000000c
[870:26:48.134,000] <err> os:      a4: 0000000080016b37    t4: 0000000000000020
[870:26:48.134,000] <err> os:      a5: 0000000000000063    t5: aaaaaaaaaaaaaaaa
[870:26:48.134,000] <err> os:      a6: 000000000000000a    t6: aaaaaaaaaaaaaaaa
[870:26:48.134,000] <err> os:      a7: 0000000080016b36
[870:26:48.134,000] <err> os:      sp: 0000000080019870
[870:26:48.134,000] <err> os:      ra: 0000000080008b86
[870:26:48.134,000] <err> os:    mepc: 000000008000ddd2
[870:26:48.134,000] <err> os: mstatus: 0000000a00001880
[870:26:48.134,000] <err> os: 
[870:26:48.134,000] <err> os: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
[870:26:48.134,000] <err> os: Current thread: 0x80016688 (shell_uart)
[870:26:48.144,000] <err> os: Halting system

and using a non-sensor device in the command will cause a hang:

uart:~$ sensor get sensor@0 ambient_temp 
channel idx=13 ambient_temp shift=11 num_samples=1 value=9220133425350000000ns (1234.005677)
uart:~$ sensor get uart@10000000 ambient_temp
<hang>

This PR fixes that:

uart:~$ sensor get sensor@0 current
channel idx=33 current shift=6 num_samples=1 value=9220133425350000000ns (33.999999)
uart:~$ sensor get sensor@0 ambient_temp
channel idx=13 ambient_temp shift=11 num_samples=1 value=9220133425350000000ns (1234.005677)
uart:~$ sensor get uart@10000000 ambient_temp
Device is not a sensor (uart@10000000)
uart:~$ sensor attr_get uart@10000000 ambient_temp
Device is not a sensor (uart@10000000)
uart:~$ sensor attr_set uart@10000000 ambient_temp
Device is not a sensor (uart@10000000)
uart:~$ sensor attr_set uart@10000000 sampling_frequency
Device is not a sensor (uart@10000000)
uart:~$

Fixes #72838

ycsin · 2024-05-15T16:47:45Z

EDIT: this seems to be intended after #61022

The output of the get commands seems weird to me (no reply or unexpected channel idx for certain channel, i.e.: 0, 1, 2, 4, 5, 6), but I'm not sure if it is expected.

Can be reproduced in this PR with:

west build -p auto -b qemu_riscv64 zephyr/tests/drivers/sensor/sensor_shell/ -t run

uart:~$ sensor get sensor@0 0
channel idx=3 accel_xyz shift=0 num_samples=0 value=4294967295ns, (-0.000000, -0.000000, -0.000000)
uart:~$ sensor get sensor@0 1
uart:~$ sensor get sensor@0 2
uart:~$ sensor get sensor@0 3
channel idx=3 accel_xyz shift=31 num_samples=1 value=9220133425350000000ns, (3.000000, 1.000000, -1139.000000)
uart:~$ sensor get sensor@0 4
channel idx=7 gyro_xyz shift=0 num_samples=0 value=4294967295ns, (-0.000000, -0.000000, -0.000000)
uart:~$ sensor get sensor@0 5
uart:~$ sensor get sensor@0 6
uart:~$ sensor get sensor@0 7
channel idx=7 gyro_xyz shift=31 num_samples=1 value=9220133425350000000ns, (7.000000, 1.000000, -1139.000000)
uart:~$ sensor get sensor@0 8
channel idx=11 magn_xyz shift=0 num_samples=0 value=4294967295ns, (-0.000000, -0.000000, -0.000000)
uart:~$ sensor get sensor@0 9
uart:~$ sensor get sensor@0 10
uart:~$ sensor get sensor@0 11
channel idx=11 magn_xyz shift=31 num_samples=1 value=9220133425350000000ns, (11.000000, 1.000000, -1139.000000)
uart:~$ sensor get sensor@0 12
channel idx=12 die_temp shift=4 num_samples=1 value=9220133425350000000ns (12.999999)
uart:~$ sensor get sensor@0 13
channel idx=13 ambient_temp shift=4 num_samples=1 value=9220133425350000000ns (13.999999)
uart:~$ sensor get sensor@0 14
channel idx=14 press shift=4 num_samples=1 value=9220133425350000000ns (14.999999)
uart:~$ sensor get sensor@0 15
channel idx=15 prox num_samples=0 value=4294967295ns (is_near = 1)
uart:~$ sensor get sensor@0 16
channel idx=16 humidity shift=5 num_samples=1 value=9220133425350000000ns (16.999999)
uart:~$ sensor get sensor@0 17
channel idx=17 light shift=5 num_samples=1 value=9220133425350000000ns (17.999999)
uart:~$ sensor get sensor@0 18
channel idx=18 ir shift=5 num_samples=1 value=9220133425350000000ns (18.999999)
uart:~$ sensor get sensor@0 19
channel idx=19 red shift=5 num_samples=1 value=9220133425350000000ns (19.999999)
uart:~$ sensor get sensor@0 20
channel idx=20 green shift=5 num_samples=1 value=9220133425350000000ns (20.999999)
uart:~$ sensor get sensor@0 21
channel idx=21 blue shift=5 num_samples=1 value=9220133425350000000ns (21.999999)
uart:~$ sensor get sensor@0 22
channel idx=22 altitude shift=5 num_samples=1 value=9220133425350000000ns (22.999999)
uart:~$ sensor get sensor@0 23
channel idx=23 pm_1_0 shift=5 num_samples=1 value=9220133425350000000ns (23.999999)
uart:~$ sensor get sensor@0 24
channel idx=24 pm_2_5 shift=5 num_samples=1 value=9220133425350000000ns (24.999999)
uart:~$ sensor get sensor@0 25
channel idx=25 pm_10 shift=5 num_samples=1 value=9220133425350000000ns (25.999999)
uart:~$ sensor get sensor@0 26
channel idx=26 distance shift=5 num_samples=1 value=9220133425350000000ns (26.999999)
uart:~$ sensor get sensor@0 27
channel idx=27 co2 shift=5 num_samples=1 value=9220133425350000000ns (27.999999)
uart:~$ sensor get sensor@0 28
uart:~$ sensor get sensor@0 29
channel idx=29 voc shift=5 num_samples=1 value=9220133425350000000ns (29.999999)
uart:~$ sensor get sensor@0 30
channel idx=30 gas_resistance shift=5 num_samples=1 value=9220133425350000000ns (30.999999)
uart:~$ sensor get sensor@0 31
channel idx=31 voltage shift=6 num_samples=1 value=9220133425350000000ns (31.999999)
uart:~$ sensor get sensor@0 32
uart:~$ sensor get sensor@0 33
channel idx=33 current shift=6 num_samples=1 value=9220133425350000000ns (33.999999)
uart:~$ sensor get sensor@0 34
channel idx=34 power shift=6 num_samples=1 value=9220133425350000000ns (34.999999)
uart:~$ sensor get sensor@0 35
channel idx=35 resistance shift=6 num_samples=1 value=9220133425350000000ns (35.999999)
uart:~$ sensor get sensor@0 36
channel idx=36 rotation shift=6 num_samples=1 value=9220133425350000000ns (36.999999)
uart:~$ sensor get sensor@0 37
channel idx=37 pos_dx shift=0 num_samples=0 value=4294967295ns, (-0.000000, -0.000000, -0.000000)
uart:~$ sensor get sensor@0 38
uart:~$ sensor get sensor@0 39
uart:~$ sensor get sensor@0 40
channel idx=40 rpm shift=6 num_samples=1 value=9220133425350000000ns (40.999999)
uart:~$ sensor get sensor@0 41
channel idx=41 gauge_voltage shift=6 num_samples=1 value=9220133425350000000ns (41.999999)
uart:~$ sensor get sensor@0 42
channel idx=42 gauge_avg_current shift=6 num_samples=1 value=9220133425350000000ns (42.999999)
uart:~$ sensor get sensor@0 43
channel idx=43 gauge_stdby_current shift=6 num_samples=1 value=9220133425350000000ns (43.999999)
uart:~$ sensor get sensor@0 44
channel idx=44 gauge_max_load_current shift=6 num_samples=1 value=9220133425350000000ns (44.999999)
uart:~$ sensor get sensor@0 45
channel idx=45 gauge_temp shift=6 num_samples=1 value=9220133425350000000ns (45.999999)
uart:~$ sensor get sensor@0 46
channel idx=46 gauge_state_of_charge shift=6 num_samples=1 value=9220133425350000000ns (46.999999)
uart:~$ sensor get sensor@0 47
channel idx=47 gauge_full_cap shift=6 num_samples=1 value=9220133425350000000ns (47.999999)
uart:~$ sensor get sensor@0 48
channel idx=48 gauge_remaining_cap shift=6 num_samples=1 value=9220133425350000000ns (48.999999)
uart:~$ sensor get sensor@0 49
channel idx=49 gauge_nominal_cap shift=6 num_samples=1 value=9220133425350000000ns (49.999999)
uart:~$ sensor get sensor@0 50
channel idx=50 gauge_full_avail_cap shift=6 num_samples=1 value=9220133425350000000ns (50.999999)
uart:~$ sensor get sensor@0 51
channel idx=51 gauge_avg_power shift=6 num_samples=1 value=9220133425350000000ns (51.999999)
uart:~$ sensor get sensor@0 52
channel idx=52 gauge_state_of_health shift=6 num_samples=1 value=9220133425350000000ns (52.999999)
uart:~$ sensor get sensor@0 53
channel idx=53 gauge_time_to_empty shift=6 num_samples=1 value=9220133425350000000ns (53.999999)
uart:~$ sensor get sensor@0 54
channel idx=54 gauge_time_to_full shift=6 num_samples=1 value=9220133425350000000ns (54.999999)
uart:~$ sensor get sensor@0 55
channel idx=55 gauge_cycle_count shift=0 num_samples=0 value=4294967295ns (-0.000000)
uart:~$ sensor get sensor@0 56
channel idx=56 gauge_design_voltage shift=6 num_samples=1 value=9220133425350000000ns (56.999999)
uart:~$ sensor get sensor@0 57
channel idx=57 gauge_desired_voltage shift=6 num_samples=1 value=9220133425350000000ns (57.999999)
uart:~$ sensor get sensor@0 58
channel idx=58 gauge_desired_charging_current shift=6 num_samples=1 value=9220133425350000000ns (58.999999)
uart:~$ sensor get sensor@0 59
uart:~$ sensor get sensor@0 60
uart:~$ sensor get sensor@0 61

I was hoping that I can do a for-loop in the pytest script to test the whole range of channel & attribute

ycsin · 2024-05-16T06:03:27Z

Waiting for #72833 to be merged first, then this PR can update the pytest there to validate the fix.

As new channels are added to the `enum sensor_channel`, some of the newer channel aren't updated in the whitelist of rtio decoder. Instead of specifying every channel in the list, do: 1. Verify that the `channel` is valid 2. cherry-pick the channels that require special handling, i.e. 1. `three_axis_data` 2. `byte_data` 3. `uint64_data` 3. handle the remaining `channel` in the default case as `q31_data` to make sure that all channels are handled. Updated the pytest to get channel 32, previously nothing would happen for this channel as there isn't a decoder for it, now it would return: ``` channel type=32((null)) ``` the channel name is NULL because it wasn't added to the channel name look up table in the sensor_shell.c, that is being fixed in zephyrproject-rtos#72815. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

ycsin · 2024-05-21T09:17:27Z

cc @teburd @MaureenHelm @ubieda this should be ready, PTAL, thanks

As new channels are added to the `enum sensor_channel`, some of the newer channel aren't updated in the whitelist of rtio decoder. Instead of specifying every channel in the list, do: 1. Verify that the `channel` is valid 2. cherry-pick the channels that require special handling, i.e. 1. `three_axis_data` 2. `byte_data` 3. `uint64_data` 3. handle the remaining `channel` in the default case as `q31_data` to make sure that all channels are handled. Updated the pytest to get channel 32, previously nothing would happen for this channel as there isn't a decoder for it, now it would return: ``` channel type=32((null)) ``` the channel name is NULL because it wasn't added to the channel name look up table in the sensor_shell.c, that is being fixed in zephyrproject-rtos#72815. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

As new channels are added to the `enum sensor_channel`, some of the newer channel aren't updated in the whitelist of rtio decoder. Instead of specifying every channel in the list, do: 1. Verify that the `channel` is valid 2. cherry-pick the channels that require special handling, i.e. 1. `three_axis_data` 2. `byte_data` 3. `uint64_data` 3. handle the remaining `channel` in the default case as `q31_data` to make sure that all channels are handled. Updated the pytest to get channel 32, previously nothing would happen for this channel as there isn't a decoder for it, now it would return: ``` channel type=32((null)) ``` the channel name is NULL because it wasn't added to the channel name look up table in the sensor_shell.c, that is being fixed in #72815. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

The `SENSOR_CHAN_VSHUNT` was added in zephyrproject-rtos#60717 but was never added to the `sensor_channel_name[SENSOR_CHAN_COMMON_COUNT]` table. Since the length of `sensor_channel_name` is fixed to `SENSOR_CHAN_COMMON_COUNT`, this means that the index at `SENSOR_CHAN_VSHUNT` points to `NULL`. When we use the `sensor get` command for anything bigger than `SENSOR_CHAN_VSHUNT`, we will deref that `NULL` pointer when we do `strcmp` in the for-loop of `parse_named_int`. Fix this by defining `SENSOR_CHAN_VSHUNT` in the table. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

If `CONFIG_SENSOR_INFO` is enabled, use the `sensor_info` section to validate that the argument is a sensor before using, otherwise the shell command will hang the application. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

Update the pytest script to exercise the entire `sensor_channel_name` table with `parse_named_int()`. The `gauge_desired_charging_current` is selected because it is the last one in the table before `all`, `all` is not used because `sensor get sensor@0 all` doesn't return anything. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

Instead of specifying 3 axis channels manually, use `SENSOR_CHANNEL_3_AXIS` instead. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

ycsin · 2024-05-23T03:11:42Z

Rebased to fix merge conflict, added:

sensors: shell: use SENSOR_CHANNEL_3_AXIS whenever possible (minor housekeeping)
sensors: add new channel SENSOR_CHAN_POS_DXYZ

Before sensors: add new channel SENSOR_CHAN_POS_DXYZ, none of pos_dx, pos_dy & pos_dz do not seem to give valid data:

uart:~$ sensor get sensor@0 pos_dx
channel type=37(pos_dx) index=0 shift=0 num_samples=0 value=4294967295ns, (-0.000000, -0.000000, -0.000000)
uart:~$ sensor get sensor@0 pos_dy
uart:~$ sensor get sensor@0 pos_dz

After:
(something strange with pos_dx, pos_dy & pos_dz, but pos_dxyz gives expected result, consistent with other 3-axis channels)

uart:~$ sensor get sensor@0 pos_dx
channel type=40(pos_dxyz) index=0 shift=0 num_samples=0 value=4294967295ns, (-0.000000, -0.000000, -0.000000)
uart:~$ sensor get sensor@0 pos_dy
uart:~$ sensor get sensor@0 pos_dz
uart:~$ sensor get sensor@0 pos_dxyz
channel type=40(pos_dxyz) index=0 shift=6 num_samples=1 value=9220133425350000000ns, (40.000000, 40.000000, 40.000000)

Add new channel: `SENSOR_CHAN_POS_DXYZ`, so that it is consistent with other 3-axis channels. Updated pytest, `sensor_shell` & `fake_sensor` accordingly. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

ycsin · 2024-05-23T10:31:32Z

Doc build is probably broken, the error msg is consistent in a few PRs

As new channels are added to the `enum sensor_channel`, some of the newer channel aren't updated in the whitelist of rtio decoder. Instead of specifying every channel in the list, do: 1. Verify that the `channel` is valid 2. cherry-pick the channels that require special handling, i.e. 1. `three_axis_data` 2. `byte_data` 3. `uint64_data` 3. handle the remaining `channel` in the default case as `q31_data` to make sure that all channels are handled. Updated the pytest to get channel 32, previously nothing would happen for this channel as there isn't a decoder for it, now it would return: ``` channel type=32((null)) ``` the channel name is NULL because it wasn't added to the channel name look up table in the sensor_shell.c, that is being fixed in zephyrproject-rtos#72815. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

ycsin added the bug The issue is a bug, or the PR is fixing a bug label May 15, 2024

ycsin force-pushed the pr/fix_sensor_shell branch from 22f94cb to 091f2b6 Compare May 15, 2024 16:49

ycsin marked this pull request as ready for review May 15, 2024 16:49

zephyrbot added the area: Sensors Sensors label May 15, 2024

zephyrbot requested review from avisconti, MaureenHelm, teburd, tristan-google and yperess May 15, 2024 16:49

zephyrbot assigned MaureenHelm May 15, 2024

ycsin force-pushed the pr/fix_sensor_shell branch 3 times, most recently from c046708 to 7c831b0 Compare May 16, 2024 03:03

ycsin marked this pull request as draft May 16, 2024 04:31

ycsin force-pushed the pr/fix_sensor_shell branch from 7c831b0 to 0fe04ef Compare May 16, 2024 05:50

ycsin changed the title ~~drivers: sensors: sensor_shell: fix hang, add test~~ drivers: sensors: sensor_shell: fix hang & crash May 16, 2024

ycsin force-pushed the pr/fix_sensor_shell branch from 0fe04ef to fadbdec Compare May 16, 2024 07:11

ubieda self-requested a review May 16, 2024 18:55

ycsin force-pushed the pr/fix_sensor_shell branch from fadbdec to d0a2d64 Compare May 17, 2024 03:18

zephyrbot added manifest manifest-hal_nordic manifest-hal_ambiq labels May 17, 2024

ycsin force-pushed the pr/fix_sensor_shell branch from d0a2d64 to 609dcae Compare May 17, 2024 03:21

ycsin removed manifest manifest-hal_nordic manifest-hal_ambiq labels May 17, 2024

zephyrproject-rtos deleted a comment from zephyrbot May 17, 2024

ycsin force-pushed the pr/fix_sensor_shell branch from 609dcae to 5edeefc Compare May 17, 2024 03:57

ycsin force-pushed the pr/fix_sensor_shell branch from 278f524 to 95649ef Compare May 19, 2024 04:20

ycsin force-pushed the pr/fix_sensor_shell branch from 95649ef to c984c79 Compare May 21, 2024 09:14

teburd previously approved these changes May 21, 2024

View reviewed changes

ubieda previously approved these changes May 21, 2024

View reviewed changes

ycsin added 4 commits May 23, 2024 10:34

sensor_shell: fix hang when device is not a sensor

08802c2

If `CONFIG_SENSOR_INFO` is enabled, use the `sensor_info` section to validate that the argument is a sensor before using, otherwise the shell command will hang the application. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

sensors: shell: use SENSOR_CHANNEL_3_AXIS whenever possible

2d3e9fd

Instead of specifying 3 axis channels manually, use `SENSOR_CHANNEL_3_AXIS` instead. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

ycsin dismissed stale reviews from ubieda and teburd via 5ae7f1b May 23, 2024 03:01

ycsin force-pushed the pr/fix_sensor_shell branch from c984c79 to 5ae7f1b Compare May 23, 2024 03:01

teburd previously approved these changes May 23, 2024

View reviewed changes

sensors: add new channel SENSOR_CHAN_POS_DXYZ

fd53bf7

Add new channel: `SENSOR_CHAN_POS_DXYZ`, so that it is consistent with other 3-axis channels. Updated pytest, `sensor_shell` & `fake_sensor` accordingly. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

ycsin dismissed teburd’s stale review via fd53bf7 May 23, 2024 04:15

ycsin force-pushed the pr/fix_sensor_shell branch from 5ae7f1b to fd53bf7 Compare May 23, 2024 04:15

ycsin closed this May 23, 2024

ycsin reopened this May 23, 2024

MaureenHelm approved these changes May 23, 2024

View reviewed changes

teburd approved these changes May 23, 2024

View reviewed changes

ubieda approved these changes May 23, 2024

View reviewed changes

nashif merged commit e993e99 into zephyrproject-rtos:main May 24, 2024
46 of 48 checks passed

ycsin deleted the pr/fix_sensor_shell branch May 24, 2024 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: sensors: sensor_shell: fix hang & crash #72815

drivers: sensors: sensor_shell: fix hang & crash #72815

ycsin commented May 15, 2024 •

edited

ycsin commented May 15, 2024 •

edited

ycsin commented May 16, 2024 •

edited

ycsin commented May 21, 2024

ycsin commented May 23, 2024 •

edited

ycsin commented May 23, 2024

drivers: sensors: sensor_shell: fix hang & crash #72815

drivers: sensors: sensor_shell: fix hang & crash #72815

Conversation

ycsin commented May 15, 2024 • edited

ycsin commented May 15, 2024 • edited

ycsin commented May 16, 2024 • edited

ycsin commented May 21, 2024

ycsin commented May 23, 2024 • edited

ycsin commented May 23, 2024

ycsin commented May 15, 2024 •

edited

ycsin commented May 15, 2024 •

edited

ycsin commented May 16, 2024 •

edited

ycsin commented May 23, 2024 •

edited