bluetooth: host: Dangling pointer in le_adv_recv

[stephanosio]

Describe the bug

The le_adv_recv function sets the addr field of the struct bt_le_scan_recv_info *info argument to the address of a local (stack) variable (hence, dangling pointer):

zephyr/subsys/bluetooth/host/scan.c

Line 463 in c0317fb

info->addr = &id_addr;

To Reproduce

Build tests/bluetooth/host_long_adv_recv/bluetooth.host_long_adv_recv test on native_posix with GCC 12.1 as the host compiler.

Expected behavior

No dangling pointers are used

Impact

Undefined behaviour

Logs and console output

FAILED: zephyr/subsys/bluetooth/host/CMakeFiles/subsys__bluetooth__host.dir/scan.c.obj
ccache /opt/gcc-12/bin/gcc -DKERNEL -DTC_RUNID=1e127edb3896fb51ae1b9ffe0d6cbdc4 -D_FORTIFY_SOURCE=2 -D_POSIX_C_SOURCE=200809 -D_XOPEN_SOURCE=600 -D_XOPEN_SOURCE_EXTENDED -D__ZEPHYR__=1 -I/home/stephanos/Dev/zephyrproject/zephyr/include/zephyr -I/home/stephanos/Dev/zephyrproject/zephyr/include -I/home/stephanos/Dev/zephyrproject/zephyr/twister-out/native_posix/tests/bluetooth/host_long_adv_recv/bluetooth.host_long_adv_recv/zephyr/include/generated -I/home/stephanos/Dev/zephyrproject/zephyr/soc/posix/inf_clock -I/home/stephanos/Dev/zephyrproject/zephyr/boards/posix/native_posix -I/home/stephanos/Dev/zephyrproject/zephyr/subsys/bluetooth -I/home/stephanos/Dev/zephyrproject/zephyr/subsys/testsuite/include -I/home/stephanos/Dev/zephyrproject/zephyr/subsys/testsuite/ztest/include -I/home/stephanos/Dev/zephyrproject/zephyr/subsys/testsuite/include/zephyr -I/home/stephanos/Dev/zephyrproject/zephyr/subsys/testsuite/ztest/include/zephyr -I/home/stephanos/Dev/zephyrproject/modules/crypto/tinycrypt/lib/include -Os -imacros /home/stephanos/Dev/zephyrproject/zephyr/twister-out/native_posix/tests/bluetooth/host_long_adv_recv/bluetooth.host_long_adv_recv/zephyr/include/generated/autoconf.h -ffreestanding -fno-common -g -gdwarf-4 -fdiagnostics-color=always -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wno-main -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -Werror -fno-asynchronous-unwind-tables -fno-pie -fno-pic -fno-reorder-functions -fno-defer-pop -fmacro-prefix-map=/home/stephanos/Dev/zephyrproject/zephyr/tests/bluetooth/host_long_adv_recv=CMAKE_SOURCE_DIR -fmacro-prefix-map=/home/stephanos/Dev/zephyrproject/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/home/stephanos/Dev/zephyrproject=WEST_TOPDIR -ffunction-sections -fdata-sections -m32 -include /home/stephanos/Dev/zephyrproject/zephyr/arch/posix/include/posix_cheats.h -fno-freestanding -std=c11 -MD -MT zephyr/subsys/bluetooth/host/CMakeFiles/subsys__bluetooth__host.dir/scan.c.obj -MF zephyr/subsys/bluetooth/host/CMakeFiles/subsys__bluetooth__host.dir/scan.c.obj.d -o zephyr/subsys/bluetooth/host/CMakeFiles/subsys__bluetooth__host.dir/scan.c.obj -c /home/stephanos/Dev/zephyrproject/zephyr/subsys/bluetooth/host/scan.c
/home/stephanos/Dev/zephyrproject/zephyr/subsys/bluetooth/host/scan.c: In function ‘le_adv_recv’:
/home/stephanos/Dev/zephyrproject/zephyr/subsys/bluetooth/host/scan.c:463:20: error: storing the address of local variable ‘id_addr’ in ‘*info.addr’ [-Werror=dangling-pointer=]
  463 |         info->addr = &id_addr;
      |         ~~~~~~~~~~~^~~~~~~~~~
/home/stephanos/Dev/zephyrproject/zephyr/subsys/bluetooth/host/scan.c:439:22: note: ‘id_addr’ declared here
  439 |         bt_addr_le_t id_addr;
      |                      ^~~~~~~
/home/stephanos/Dev/zephyrproject/zephyr/subsys/bluetooth/host/scan.c:439:22: note: ‘info’ declared here
cc1: all warnings being treated as errors

Environment (please complete the following information):

• OS: Ubuntu 20.04
• Toolchain: GCC 12.1 (locally compiled and installed from source)
• Commit SHA: dc3e86e

[stephanosio]

cc @jhedberg @carlescufi @joerchan @Thalley @ppryga @cvinayak

[Thalley]

Why is this an issue?

It would only be an issue if the info->addr is used afterwards, but that's not the case here.

It is true that the lifetime of the bt_le_scan_recv_info is longer than the lifetime of the info->addr, so perhaps that should be changed so that's not the case (or alternatively actually allocate the address in the bt_le_scan_recv_info instead of just a pointer).

[stephanosio]

It would only be an issue if the info->addr is used afterwards, but that's not the case here.

Then why are you setting it?

[Thalley]

It would only be an issue if the info->addr is used afterwards, but that's not the case here.

Then why are you setting it?

It is being forwarded to the upper layers in the callback.

What is happening is basically this:

struct my_struct {
    int a;
    int *b;
};

void foo(struct my_struct *ms)
{
    int local_val = 10;

    ms->b = &local_val;
    /* ms->b is valid */

    callbacks->my_callback(ms);
    /* my_callback can access and use both ms->a and ms->b */
}

void bar(void)
{
    struct my_struct ms;

    ms.a = 5;
    foo(&ms);
    /* ms->b is no longer valid */
}

Which isn't really a problem, but on the other hand isn't really great either as local_val and ms have different lifetimes/scopes.

[stephanosio]

What is happening is basically this:

Alright, if the callback, called from the le_adv_recv function, is the only place where info->addr is actually dereferenced, it should not result in an undefined behaviour.

Nevertheless, it is still "returning" the pointer to a local object to the caller of le_adv_recv, who may or may not dereference it.

I guess we can fix this in one of the following ways:

a) sanitize the info->addr before returning from le_adv_recv (i.e. set to NULL)
b) embed bt_addr_le_t info in bt_le_scan_recv_info

The latter should be preferred if there are no negative side effects (e.g. significant footprint increase).

[Thalley]

What is happening is basically this:

Alright, if the callback, called from the le_adv_recv function, is the only place where info->addr is actually dereferenced, it should not result in an undefined behaviour.

Nevertheless, it is still "returning" the pointer to a local object to the caller of le_adv_recv, who may or may not dereference it.

I guess we can fix this in one of the following ways:

a) sanitize the info->addr before returning from le_adv_recv (i.e. set to NULL) b) embed bt_addr_le_t info in bt_le_scan_recv_info

The latter should be preferred if there are no negative side effects (e.g. significant footprint increase).

Agreed. The struct in question is a shortlived-stack allocated variable and the increased footprint of embedding the address into the struct isn't a problem. It is, however, different than how we generally report Bluetooth addresses in callback structures.

@stephanosio Did you add any additional warnings when you built? Because I've been building for native_posix at least once a week for years and never seen this warning, and this code is somewhat old.

[stephanosio]

@stephanosio Did you add any additional warnings when you built? Because I've been building for native_posix at least once a week for years and never seen this warning, and this code is somewhat old.

You are not going to see the above warning unless you are on a bleeding edge distro with the GCC 12.1 as the default host compiler.

To Reproduce
Build tests/bluetooth/host_long_adv_recv/bluetooth.host_long_adv_recv test on native_posix with GCC 12.1 as the host compiler.

[Thalley]

You are not going to see the above warning unless you are on a bleeding edge distro with the GCC 12.1 as the default host compiler.

I just compiled with GCC 12.1 (I am running Arch Linux) and I do not get this warning :S

Edit: I actually get it for that specific test, but not when building other applications.
Edit 2: I actually do get it for some others :)

[Thalley]

a) sanitize the info->addr before returning from le_adv_recv (i.e. set to NULL)
b) embed bt_addr_le_t info in bt_le_scan_recv_info

The latter should be preferred if there are no negative side effects (e.g. significant footprint increase).

Option a) also has the benefit of not changing the (stable and widely used) API.
I suggest that we either do option a) (tested to work already), or that we allocate the address at the same time as the info object so that they share lifetimes.

[joerchan]

For b) I think this should be done more thoroughly if decided to do it. There are other structs that does the same thing as well, e.g. bt_le_per_adv_sync_term_info.

or that we allocate the address at the same time as the info object so that they share lifetimes.

IMO this is the best option.