scan-build errors found during 2019.07 testing

[cladmi]

Description

I am running scan-build checking for the 2019.07 release RIOT-OS/Release-Specs#128

I will try to list the issues I found here.

By running the scan-build command manually, you get the webpage that gives the path taken by the analyzer to show the error. Which is good to have context.

# Same boards as `RIOT` building for `llvm`
BOARDS="iotlab-m3 native nrf52dk mulle nucleo-f401re samr21-xpro slstk3402a"

export BUILD_IN_DOCKER=1
export TOOLCHAIN=llvm


# Cannot currently be done through `RIOT_MAKEFILES_GLOBAL_POST` with `docker`
# So hack it
echo 'CFLAGS += -Wno-deprecated-declarations' >> Makefile.include

for board in ${BOARDS}
do
    ./dist/tools/compile_and_test_for_board/compile_and_test_for_board.py --compile-targets scan-build-analyze --no-test . ${board} || true
done

The parsed result is available here: https://ci-ilab.imp.fu-berlin.de/job/RIOT%20scan-build-analyze/18/

How should issues be handled?

• Is there a way in RIOT to define that an attribute is __nonnull ? I do not see it used but is defined in sys/cdefs.h.
• Should we always disable -Wno-deprecated-declarations when running scan-build ? Or a way to have a supported CFLAGS_APPEND.
• Should value is never read be disabled or correctly handled?

List of found issues:

TODO

• list all (except packages for the moment)
• sort real problems

Reported files

• RIOT/sys/net/application_layer/nanocoap/nanocoap.c

  Line 307 in 14fe8f2

  uint32_t blkopt = _decode_uint(data_start, option_len);

  TOOLCHAIN=llvm make -C examples/nanocoap_server/ scan-build
  
  sys/net/application_layer/nanocoap/nanocoap.c:307:23: warning: 2nd function call argument is an
  uninitialized value
      uint32_t blkopt = _decode_uint(data_start, option_len);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  sys/net/application_layer/nanocoap/nanocoap.c:554:14: warning: The left expression of the compou
  nd assignment is an uninitialized value. The computed value will also be garbage
          *buf |= (val << shift);
          ~~~~ ^
  sys/net/application_layer/nanocoap/nanocoap.c:557:14: warning: The left expression of the compou
  nd assignment is an uninitialized value. The computed value will also be garbage
          *buf |= (13 << shift);
          ~~~~ ^
  sys/net/application_layer/nanocoap/nanocoap.c:561:14: warning: The left expression of the compou
  nd assignment is an uninitialized value. The computed value will also be garbage
          *buf |= (14 << shift);
          ~~~~ ^
  4 warnings generated.
  sys/net/application_layer/nanocoap/sock.c:147:17: warning: Value stored to 'res' is never read
                  res = sock_udp_send(&sock, buf, res, &remote);
                  ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  1 warning generated.
  

  Error handling is done on option_len but as there was no length,
  it is used not initialized.

• RIOT/cpu/sam0_common/include/periph_cpu_common.h

  Line 395 in 14fe8f2

  MCLK->APBBMASK.reg |= (1 << (id + 7));

  BOARD=samr21-xpro TOOLCHAIN=llvm make -C examples/hello-world/ scan-build
  
  cpu/sam0_common/include/periph_cpu_common.h:395:46: warning: The result of the
      left shift is undefined due to shifting by '255',
      which is greater or equal to the width of type 'unsigned long'
      PM->APBCMASK.reg |= (PM_APBCMASK_SERCOM0 << id);
  

  The default case returns -1 for an uint8_t which also causes invalid C code.
  Could be replaced by a compile time error.

• RIOT/drivers/ata8520e/ata8520e.c

  Line 203 in 14fe8f2

  uint8_t atmel = spi_transfer_byte(SPIDEV, CSPIN, true, 0);

  TOOLCHAIN=llvm make -C tests/driver_ata8520e/ scan-build
  
  drivers/ata8520e/ata8520e.c: atmel/sigfox/sigfox2 are unused
  

  Could be (void) in an else statement.

• RIOT/drivers/bmp180/bmp180.c

  Line 120 in 14fe8f2

  _compute_b5(dev, ut, &b5);

  TOOLCHAIN=llvm make -C tests/driver_bmp180/ scan-build
  
  drivers/bmp180/bmp180.c:120:5: warning: 2nd function call argument is an uninitialized value _compute_b5(dev, ut, &b5);
  

  As the function does not check the return value, _compute_b5 can use uninitialized value in case of error.
  Also it currently acquires 2 times the i2c for _read_ut.

• RIOT/drivers/nrf24l01p/nrf24l01p.c

  Line 644 in 14fe8f2

  return pwr;

  TOOLCHAIN=llvm make -C tests/driver_nrf24l01p_lowlevel scan-build
  
  drivers/nrf24l01p/nrf24l01p.c:644:5: warning: Undefined or garbage value returned to caller 'return pwr;'
  

  Would need an enum I guess instead of hardwriten values and generic type.

• RIOT/drivers/tmp006/tmp006.c

  Line 248 in 14fe8f2

  if (!drdy) {

  TOOLCHAIN=llvm make -C tests/driver_tmp006/ scan-build
  
  drivers/tmp006/tmp006.c:248:9: warning: Branch condition evaluates to a garbage value
      if (!drdy) {
          ^~~~~
  drivers/tmp006/tmp006.c:251:5: warning: 1st function call argument is an uninitialized value
      tmp006_convert(rawvolt, rawtemp,  &tamb, &tobj);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/tmp006/tmp006.c:251:5: warning: 2nd function call argument is an uninitialized value
      tmp006_convert(rawvolt, rawtemp,  &tamb, &tobj);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  

  As the function does not check the return value of tmp006_read in case of error,
  it would result in using drdy not initialized.
  There is even some drdy error checking, but what if the function failed…

• RIOT/pkg/lwip/contrib/netdev/lwip_netdev.c

  Line 203 in 14fe8f2

  iolist_t iolist[count];

  TOOLCHAIN=llvm make -C tests/lwip_sock_ip/ scan-build
  
  pkg/lwip/contrib/netdev/lwip_netdev.c:203:5: warning: Declared variable-length array (VLA) has zero size
      iolist_t iolist[count];
      ^~~~~~~~~~~~~~~ ~~~~~
  1 warning generated.
  pkg/lwip/contrib/sock/lwip_sock.c:522:14: warning: Dereference of null pointer (loaded from variable 'conn')
      else if (*conn != NULL) {
               ^~~~~
  

  LL_COUNT(p, q, count); may return a 0 value for count.
  It seems that indeed conn can be NULL before being de-referenced

• core/include/rmutex.h used by

  RIOT/sys/net/gnrc/netif/gnrc_netif.c

  Line 23 in 14fe8f2

  #include "net/gnrc.h"

  TOOLCHAIN=llvm make -C tests/netdev_test/ scan-build
  
  In file included from sys/net/gnrc/netif/gnrc_netif.c:23:
  In file included from sys/include/net/gnrc.h:294:
  In file included from sys/include/net/gnrc/netif.h:56:
  core/include/rmutex.h:79:13: warning: Dereference of null pointer (loaded from variable 'rmutex')
      *rmutex = empty_rmutex;
       ~~~~~~ ^
  

  There is an assert(netif != NULL) but still assumes that &netif->mutex can be NULL.
  The rmutex function could be decorated with a __nonnull attribute completely.

• RIOT/sys/hashes/sha3.c

  Line 427 in 14fe8f2

  blockSize = 0;

  TOOLCHAIN=llvm make -C tests/unittests/ scan-build
  
  sys/hashes/sha3.c:427:13: warning: Value stored to 'blockSize' is never read
      blockSize = 0;
  

  The value is never used. This looks indeed not used. Not sure if it was meant
  or if it is an initialization that should be outside the loop.

• RIOT/sys/hashes/sha256.c

  Line 80 in 14fe8f2

  dst[i] = src[i + 3];

  TOOLCHAIN=llvm make -C tests/unittests/ scan-build
  
  sys/hashes/sha256.c:80:20: warning: Assigned value is garbage or undefined
      dst[i] = src[i + 3];
             ^ ~~~~~~~~~~
  

  The compiler has no idea that src is of the right length.
  A more precise type definition could help, would be good to find the correct way of declaring this for the compiler.

• RIOT/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c

  Line 232 in 14fe8f2

  const ipv6_hdr_t *hdr = pkt->next->data;

  TOOLCHAIN=llvm make -C tests/gnrc_rpl_srh/ scan-build
  
  sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c:232:23: warning: Value stored to 'hdr' during its initialization is never read
      const ipv6_hdr_t *hdr = pkt->next->data;
                            ^~~   ~~~~~~~~~~~~~~~
  

  Initialized even if DEBUG is not set.

• sys/include/net/addr.h through

  RIOT/sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c

  Line 23 in 14fe8f2

  #include "net/ipv6/hdr.h"

  TOOLCHAIN=llvm make -C examples/gnrc_networking scan-build
  
  In file included from sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c:23:
  In file included from sys/include/net/ipv6/hdr.h:27:
  sys/include/net/ipv6/addr.h:395:13: warning: Array access (via field 'u8') results in a null pointer dereference
      return (addr->u8[0] == 0xff);
                  ^~~~~~~~~~~
  

  Looks like it assumes that dst can be NULL. An assert or __nonnull attribute should help.

• RIOT/sys/net/application_layer/uhcp/uhcpc.c

  Line 40 in 14fe8f2

  int res = sock_udp_create(&sock, &local, NULL, 0);

  TOOLCHAIN=llvm make -C examples/gnrc_border_router/ scan-build
  
  sys/net/application_layer/uhcp/uhcpc.c:40:9: warning: Value stored to 'res' during its initialization is never read
      int res = sock_udp_create(&sock, &local, NULL, 0);
              ^~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  

  Value is obviously ignored.

• RIOT/sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c

  Line 1007 in 14fe8f2

  assert(udp->size >= sizeof(udp_hdr_t));

  TOOLCHAIN=llvm make -C examples/gnrc_border_router/ scan-build
  
  sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c:1007:20: warning: Access to field 'size' results
  in a dereference of a null pointer (loaded from variable 'udp')
              assert(udp->size >= sizeof(udp_hdr_t));
                     ^~~~~~~~~
  ssert.h:106:24: note: expanded from macro 'assert'
  #define assert(cond) ((cond) ? (void)0 : core_panic(PANIC_ASSERT_FAIL, assert_crash_message))
                         ^~~~
  1 warning generated.
  

  The function is so big that the reported analysis is on 300 lines.

• RIOT/sys/net/gnrc/link_layer/gomach/gomach_internal.c

  Line 786 in 14fe8f2

  own_id = 0;

  TOOLCHAIN=llvm make -C examples/gnrc_networking_mac/ scan-build
  
  sys/net/gnrc/link_layer/gomach/gomach_internal.c:786:5: warning: Value stored to 'own_id' is never read
      own_id = 0;
      ^        ~
  sys/net/gnrc/link_layer/gomach/gomach_internal.c:605:13: warning: 3rd function call argument is an uninitialized value
          if (memcmp(&netif->mac.rx.slosch_list[i].node_addr.addr,
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  sys/net/gnrc/link_layer/gomach/gomach_internal.c:638:13: warning: 3rd function call argument is an uninitialized value
          if (memcmp(&netif->mac.rx.check_dup_pkt.last_nodes[i].node_addr.addr,
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  

  Minor one indeed set just after, could be removed.
  The other one is a quite complex analysis, should have focused review. 101 steps

• RIOT/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c

  Line 232 in 14fe8f2

  const ipv6_hdr_t *hdr = pkt->next->data;

  TOOLCHAIN=llvm make -C examples/gnrc_networking_mac/ scan-build
  
  sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c:232:23: warning: Value stored to 'hdr' during its initialization is never read
      const ipv6_hdr_t *hdr = pkt->next->data;
                        ^~~   ~~~~~~~~~~~~~~~
  

  Value not used when debug disabled.

• RIOT/sys/net/gnrc/network_layer/ipv6/nib/nib.c

  Line 442 in 14fe8f2

  uint32_t next_ra_delay = random_uint32_range(0, NDP_MAX_RA_DELAY);

  TOOLCHAIN=llvm make -C examples/gnrc_networking_mac/ scan-build
  
  sys/net/gnrc/network_layer/ipv6/nib/nib.c:442:14: warning: Value stored to 'next_ra_delay' during its initialization is never read
      uint32_t next_ra_delay = random_uint32_range(0, NDP_MAX_RA_DELAY);
             ^~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  sys/net/gnrc/network_layer/ipv6/nib/nib.c:498:9: warning: Value stored to 'nce' is never read
      nce = _nib_onl_get(&ipv6->src, netif->pid);
      ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  

  Is it finding that the branches using the values are not reachable ?

• RIOT/sys/net/gnrc/network_layer/sixlowpan/gnrc_sixlowpan.c

  Line 74 in 14fe8f2

  if ((ptr->next) && (ptr->next->type == GNRC_NETTYPE_NETIF)) {

  TOOLCHAIN=llvm make -C tests/gnrc_sixlowpan_frag scan-build
  
  sys/net/gnrc/network_layer/sixlowpan/gnrc_sixlowpan.c:74:13: warning: Access to field 'next' results in a dereference of a null pointer (loaded from variable 'ptr')
      if ((ptr->next) && (ptr->next->type == GNRC_NETTYPE_NETIF)) {
          ^~~~~~~~~~~
  

  Long path of pointers that could be said __nonnull.

• RIOT/sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c

  Line 1007 in 14fe8f2

  assert(udp->size >= sizeof(udp_hdr_t));

  TOOLCHAIN=llvm make -C examples/gnrc_networking_mac scan-build
  
  sys/net/gnrc/network_layer/sixlowpan/iphc/gnrc_sixlowpan_iphc.c:1007:20: warning: Access to field 'size' results
  in a dereference of a null pointer (loaded from variable 'udp')
              assert(udp->size >= sizeof(udp_hdr_t));
                   ^~~~~~~~~
  

  It is after a long long analysis. 400 lines function.

• RIOT/sys/net/gnrc/sock/ip/gnrc_sock_ip.c

  Line 174 in 14fe8f2

  (sock->remote.family != AF_UNSPEC)) {

  TOOLCHAIN=llvm make -C tests/gnrc_sock_ip/ scan-build
  
  sys/net/gnrc/sock/ip/gnrc_sock_ip.c:174:10: warning: Dereference of null pointer
      (sock->remote.family != AF_UNSPEC)) {
       ^~~~~~~~~~~~~~~~~~~
  

  This really looks like a case where it is NULL, if I can understand
  the number of branches.

• RIOT/sys/net/routing/nhdp/iib_table.c

  Line 717 in 14fe8f2

  LL_DELETE(base_entry->two_hop_set_head, th_entry);

  TOOLCHAIN=llvm make -C tests/nhdp scan-build
  
  sys/net/routing/nhdp/iib_table.c:717:5: warning: Use of memory after it is freed
      LL_DELETE(base_entry->two_hop_set_head, th_entry);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  sys/include/utlist.h:387:5: note: expanded from macro 'LL_DELETE'
      LL_DELETE2(head,del,next)
      ^~~~~~~~~~~~~~~~~~~~~~~~~
  sys/include/utlist.h:394:12: note: expanded from macro 'LL_DELETE2'
      (head)=(head)->next;
             ^~~~~~~~~~~~
  sys/net/routing/nhdp/iib_table.c:717:5: warning: Use of memory after it is freed
      LL_DELETE(base_entry->two_hop_set_head, th_entry);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  sys/include/utlist.h:387:5: note: expanded from macro 'LL_DELETE'
      LL_DELETE2(head,del,next)
      ^~~~~~~~~~~~~~~~~~~~~~~~~
  sys/include/utlist.h:397:12: note: expanded from macro 'LL_DELETE2'
      while (_tmp->next && (_tmp->next != (del))) {
  

  Use after free, this seems bad

• RIOT/sys/net/sock/sock_util.c

  Line 154 in 14fe8f2

  char *hostend = hoststart;

  TOOLCHAIN=llvm make -C tests/unittests/ scan-build
  
  sys/net/sock/sock_util.c:154:11: warning: Value stored to 'hostend' during its initialization is never read
      char *hostend = hoststart;
                ^~~~~~~   ~~~~~~~~~
  

  Initialization could be removed it is never used.

• RIOT/sys/posix/sockets/posix_sockets.c

  Line 186 in 14fe8f2

  assert((ep->family == AF_INET) || (ep->family == AF_INET6));

  TOOLCHAIN=llvm make -C examples/posix_sockets/ scan-build
  
  sys/posix/sockets/posix_sockets.c:186:24: warning: The left operand of '==' is a garbage value
      assert((ep->family == AF_INET) || (ep->family == AF_INET6));
              ~~~~~~~~~~ ^
  

  Not correctly handled having invalid socket type for 'getsockname'

• RIOT/sys/riotboot/flashwrite.c

  Line 79 in 14fe8f2

  flashpage_pos += to_copy;

  TOOLCHAIN=llvm make -C tests/riotboot_flashwrite/ scan-build
  
  /home/harter/work/git/worktree/riot_release/sys/riotboot/flashwrite.c:79:9: warning: Value stored to 'flashpage_pos' is never read
          flashpage_pos += to_copy;
          ^                ~~~~~~~
  1 warning generated.
  

  Unused value

• https:/github.com/RIOT-OS/RIOT/blob/14fe8f29e7c4aa424fce6f90fb598796b24fe063/sys/vfs/vfs_stdio.c#L61

  BOARD=mulle TOOLCHAIN=llvm make -C tests/minimal/ scan-build
  
  sys/vfs/vfs_stdio.c:61:5: warning: Value stored to 'fd' is never read
      fd = vfs_bind(STDIN_FILENO, O_RDONLY, &_stdio_ops, (void *)STDIN_FILENO);
      ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  sys/vfs/vfs_stdio.c:64:5: warning: Value stored to 'fd' is never read
      fd = vfs_bind(STDOUT_FILENO, O_WRONLY, &_stdio_ops, (void *)STDOUT_FILENO);
      ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  2 warnings generated.
  

  Mulle usess vfs by default ?
  fd is used only in assert but look like disabled…
  Maybe need a (void)fd everytime ?

• RIOT/tests/fault_handler/main.c

  Line 43 in 14fe8f2

  *((volatile int *) FORBIDDEN_ADDRESS) = 12345;

  TOOLCHAIN=llvm make -C tests/fault_handler/ scan-build
  
  tests/fault_handler/main.c:43:43: warning: Dereference of null pointer
      *((volatile int *) FORBIDDEN_ADDRESS) = 12345;
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
  ```1 warning generated.
  It is on purpose… is it possible to silent this?
  
  

• https:/github.com/RIOT-OS/RIOT/blob/14fe8f29e7c4aa424fce6f90fb598796b24fe063/tests/gnrc_netif/common.c#L78

  tests/gnrc_netif/common.c:78:9: warning: Null pointer passed as an argument to a 'nonnull' parameter
          memcpy(tmp_buffer, data, data_len);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  1 warning generated.
  

  Condition looks like it should be && instead of ||.

• https:/github.com/RIOT-OS/RIOT/blob/14fe8f29e7c4aa424fce6f90fb598796b24fe063/tests/malloc/main.c#L59

  TOOLCHAIN=llvm make -C tests/malloc/ scan-build
  
  tests/malloc/main.c:59:13: warning: Branch condition evaluates to a garbage value
          if (head->next) {
              ^~~~~~~~~~
  1 warning generated.
  

  It assumes malloc can return null which is not handled.

• RIOT/tests/nanocoap_cli/nanocli_server.c

  Line 67 in 14fe8f2

  res = sock_udp_send(&sock, buf, res, &remote);

  TOOLCHAIN=llvm make -C tests/nanocoap_cli/ scan-build
  
  tests/nanocoap_cli/nanocli_server.c:67:17: warning: Value stored to 'res' is never read
                  res = sock_udp_send(&sock, buf, res, &remote);
                  ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  1 warning generated.
  

  Value is obviously unused.

• Some other "unused value" in tests.

• ...

Versions

2019.07-RC1

[miri64]

For the gnrc_ipv6 related parts I provided #12274.

[miri64]

Sorry, wasn't careful enough about my wording in #12274.

[cladmi]

Slap github :D