gnrc: fix several issues found with LLVM's static code analyzer #10350

miri64 · 2018-11-08T13:59:59Z

Contribution description

Fixes a few issues that were found when checking gnrc_networking with LLVM's static code analyzer.

Testing procedure

Execute make -C examples/gnrc_networking scan-build. It should pass. For functionality tests I recommend to use the a subset of the tests outlined in the release specifications.

Issues/PRs references

None

bergzand · 2018-11-08T14:44:29Z

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

@@ -240,7 +240,7 @@ int gnrc_ipv6_nib_get_next_hop_l2addr(const ipv6_addr_t *dst,
                gnrc_netif_acquire(netif);
            }
            node = _nib_onl_get(&route.next_hop,
-                                (netif == NULL) ? netif->pid : 0);
+                                (netif != NULL) ? netif->pid : 0);


This was an actual bug waiting to happen?

I guess we were lucky (or rather unlucky) that this line never really was hit.

cladmi

I do not have time to review before the week end.

But for the two real bug fixes in nib.c and nib_pl.c files, I agree with the changes (only reviewed not tested).

cladmi · 2018-11-08T14:23:34Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-arsm.c

@@ -517,6 +517,9 @@ static inline uint32_t _exp_backoff_retrans_timer(uint8_t ns_sent,
    uint32_t tmp = random_uint32_range(NDP_MIN_RANDOM_FACTOR,
                                       NDP_MAX_RANDOM_FACTOR);

+    /* this is a given since ns_sent is only incremented in _probe_nbr when
+     * it is less than (NDP_MAX_NS_NUMOF + 2) */
+    assert(ns_sent < 32);
    /* backoff according to  https://tools.ietf.org/html/rfc7048 with
     * BACKOFF_MULTIPLE == 2 */
    tmp = ((1 << ns_sent) * retrans_timer * tmp) / US_PER_MS;


For 16bit/8bit platforms, should not this 1 also be a uint32_t or even uint64_t as NDP_MAX_NS_NUMOF can be 17 ?

I could be wrong, but this sounds like a separate issue not directly related to this PR.

For 16bit/8bit platforms, should not this 1 also be a uint32_t or even uint64_t as NDP_MAX_NS_NUMOF can be 17 ?

Huh, I don't get your statement... tmp is an uint32_t. 17 + 2 is on all platforms < 32.

Or do you mean, the 1 should actually be 1UL?

(if it is the latter I agree with @bergzand: this is a matter of a separate PR)

@bergzand it is not related to silencing scan-build but has been revealed by running the static analyzer. And unfortunately we do not have llvmsupport with 8/16bit platforms.
If it is silenced, it would be forgotten.

@miri64 yes I meant the 1 should be converted to an uint32, not sure of the size of 1UL in our 8 and 16bit platform and in general ((1 << ns_sent) * retrans_timer * tmp) should not overflow in its type.

@cladmi things can be fixed in a separate PR while this PR is open ;-) and thus not be forgotten. Since I now understand your remark better, I will do that as soon as I have a keyboard under my fingers.

@cladmi see #10369

miri64 · 2018-11-08T19:44:59Z

Fixed the issue with newlines in the shell command.

miri64 · 2018-11-09T14:43:39Z

See #10358 for the release critical parts

miri64 · 2018-11-09T15:17:07Z

Rebased to current master and removed commits already merged with #10358.

miri64

Rebased to current master

miri64 · 2019-01-22T08:51:41Z

sys/net/gnrc/pktbuf_static/gnrc_pktbuf_static.c

@@ -382,7 +382,7 @@ static gnrc_pktsnip_t *_create_snip(gnrc_pktsnip_t *next, const void *data, size
        }
    }
    _set_pktsnip(pkt, next, _data, size, type);
-    if (data != NULL) {
+    if ((data != NULL) && (size > 0)) {


This was fixed in #10786 as well.

aabadie · 2019-02-12T10:29:28Z

I wanted to see what are the remaining PR that needs a backport and found this one. It's opened for quite some time now, so what's the status ? Should it be backported to 2019.01-branch ?

miri64 · 2019-02-12T11:02:59Z

There was some unrelated discussion in #10350 (comment) which was then fixed in #10369 (which was also never merged), other than that I believe this PR is ready to ~~be merged~~ be reviewed.

aabadie · 2019-02-12T12:14:55Z

@cladmi, since you already started to review this, would you mind to have a second look/test and eventually give an ACK ?

miri64 · 2019-02-12T17:07:40Z

Rebased to current master to resolve conflicts.

miri64 · 2019-02-12T17:21:09Z

I piggy-backed some cppcheck suppressions for warnings that Travis were complaining about.

aabadie · 2019-02-13T08:27:24Z

After looking at it again, it seems that this PR only contains minor things. I propose that we postpone it and remove the "needs backport" label. @miri64, @cladmi, what do you think ?

aabadie · 2019-02-13T09:01:55Z

Postponed and removed the 'needs backport' label.

cladmi

The changes looks good, but it looks like the one for the command line handling could be changing the output. Is there a way to test this ?

cladmi · 2019-03-26T19:52:51Z

sys/shell/commands/sc_gnrc_netif.c

@@ -379,19 +378,16 @@ static void _netif_list_ipv6(ipv6_addr_t *addr, uint8_t flags)
                break;
        }
    }
-    line_thresh = _newline(0U, line_thresh);
+    puts("");


These seems they would change the output, as the previous version looks like its printing \n (many spaces) .
Not sure if it is wanted to replace it with only \n.

Same goes here.

cladmi · 2019-03-26T19:54:01Z

sys/shell/commands/sc_gnrc_netif.c

    if ((ipv6_addr_is_multicast(addr))) {
        char addr_str[IPV6_ADDR_MAX_STR_LEN];
        ipv6_addr_to_str(addr_str, addr, sizeof(addr_str));
-        printf("inet6 group: %s", addr_str);
+        printf("inet6 group: %s\n", addr_str);


This changes from always printing \n (many spaces) to sometime only printing a new line.
Not sure if it is wanted.

Well the line is always too long and int should be broken after one group in any case.

miri64 · 2019-03-26T20:02:33Z

The changes looks good, but it looks like the one for the command line handling could be changing the output. Is there a way to test this ?

Compile and run gnrc_networking and check if the output changed for ifconfig ;-)

The variable is without usage reassigned with `dodag->parent` below.

For the source address compression it was already done the same way in 12bcc46

miri64 · 2019-06-20T14:25:08Z

Rebased and ping??!? Bug fixes. Mini, tiny low-hanging fruit bug-fixes, that want merging!

kaspar030

LGTM.

next time better keep simple assert additions seperate please!

miri64 · 2019-06-20T14:34:00Z

next time better keep simple assert additions seperate please!

Please clarify.

kaspar030 · 2019-06-20T14:36:33Z

Please clarify.

Ah, I looked at the diff, and saw some new "asserts()". Those I'd probably ACK very fast.
Changes to the output, not initializing some variables need deeper looking. So that would've been candidates for a differrent PR.

miri64 · 2019-06-20T14:42:15Z

Ok, so you meant separate PR, because I was confused, since the assertion fixes are in separate commits.

The indentation of `ifconfig` is currently broken, due to an attempt to fix some `scan-build` errors.

…sion shell_commands: fix regression to ifconfig introduced in #10350

The indentation of `ifconfig` is currently broken, due to an attempt to fix some `scan-build` errors.

miri64 added this to the Release 2018.10 milestone Nov 8, 2018

miri64 requested review from cladmi and bergzand November 8, 2018 14:00

miri64 mentioned this pull request Nov 8, 2018

Release 2018.10 - RC2 RIOT-OS/Release-Specs#81

Closed

54 tasks

bergzand reviewed Nov 8, 2018

View reviewed changes

cladmi reviewed Nov 8, 2018

View reviewed changes

miri64 mentioned this pull request Nov 9, 2018

gnrc_ipv6_nib: fix several issues found with LLVM's static code analyzer (only critical) #10358

Merged

miri64 removed this from the Release 2018.10 milestone Nov 9, 2018

miri64 mentioned this pull request Nov 9, 2018

gnrc_ipv6_nib: fix several issues found with LLVM's static code analyzer (only critical) [backport 2018.10] #10359

Merged

miri64 force-pushed the gnrc/fix/static-analyzis branch from eb8cd95 to 6e89c62 Compare November 9, 2018 15:16

miri64 mentioned this pull request Nov 12, 2018

gnrc_ipv6_nib/arsm: ensure proper int width in backoff calculation #10369

Merged

miri64 force-pushed the gnrc/fix/static-analyzis branch from 6e89c62 to 331d3e3 Compare January 22, 2019 08:57

miri64 commented Jan 22, 2019

View reviewed changes

miri64 mentioned this pull request Jan 22, 2019

gnrc/pktbuf: avoid memcpy if size <= 0 #10786

Merged

miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jan 29, 2019

aabadie added this to the Release 2019.01 milestone Feb 12, 2019

miri64 mentioned this pull request Feb 12, 2019

gnrc_ipv6_nib/arsm: ensure proper int width in backoff calculation [backport 2019.01] #11007

Merged

miri64 force-pushed the gnrc/fix/static-analyzis branch from 331d3e3 to b443a6b Compare February 12, 2019 17:07

aabadie modified the milestones: Release 2019.01, Release 2019.04 Feb 13, 2019

aabadie removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Feb 13, 2019

cladmi reviewed Mar 26, 2019

View reviewed changes

miri64 modified the milestones: Release 2019.04, Release 2019.07 May 7, 2019

miri64 added 4 commits June 20, 2019 16:18

gnrc_ipv6_nib: assure netif != NULL in route info callback

fa95cc8

gnrc_rpl_dodag: remove new_best assignment at initialization

3edb52d

The variable is without usage reassigned with `dodag->parent` below.

shell_commands: gnrc_netif: fix _newline() handling

2cb0ccc

gnrc_sixlowpan_iphc: assure ctx != NULL for dst compression

3ee75c3

For the source address compression it was already done the same way in 12bcc46

miri64 force-pushed the gnrc/fix/static-analyzis branch from 0a5bb10 to 3ee75c3 Compare June 20, 2019 14:23

kaspar030 added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Jun 20, 2019

kaspar030 approved these changes Jun 20, 2019

View reviewed changes

miri64 merged commit d47ac80 into RIOT-OS:master Jun 20, 2019

miri64 deleted the gnrc/fix/static-analyzis branch June 20, 2019 14:53

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 26, 2019

shell_commands: fix regression to ifconfig introduced in RIOT-OS#10350

299c1a2

The indentation of `ifconfig` is currently broken, due to an attempt to fix some `scan-build` errors.

miri64 mentioned this pull request Jun 26, 2019

shell_commands: fix regression to ifconfig introduced in #10350 #11746

Merged

cladmi added a commit that referenced this pull request Jun 26, 2019

Merge pull request #11746 from miri64/shell_commands/fix/10350-regres…

76e9d08

…sion shell_commands: fix regression to ifconfig introduced in #10350

fjmolinas pushed a commit to fjmolinas/RIOT that referenced this pull request Aug 4, 2019

shell_commands: fix regression to ifconfig introduced in RIOT-OS#10350

2c668a0

The indentation of `ifconfig` is currently broken, due to an attempt to fix some `scan-build` errors.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gnrc: fix several issues found with LLVM's static code analyzer #10350

gnrc: fix several issues found with LLVM's static code analyzer #10350

miri64 commented Nov 8, 2018

bergzand Nov 8, 2018

miri64 Nov 8, 2018

cladmi left a comment

cladmi Nov 8, 2018

bergzand Nov 8, 2018

miri64 Nov 8, 2018

miri64 Nov 8, 2018

miri64 Nov 8, 2018

cladmi Nov 12, 2018

miri64 Nov 12, 2018

miri64 Nov 12, 2018

miri64 commented Nov 8, 2018

miri64 commented Nov 9, 2018

miri64 commented Nov 9, 2018

miri64 left a comment

miri64 Jan 22, 2019

aabadie commented Feb 12, 2019

miri64 commented Feb 12, 2019 •

edited

Loading

aabadie commented Feb 12, 2019

miri64 commented Feb 12, 2019

miri64 commented Feb 12, 2019 •

edited

Loading

aabadie commented Feb 13, 2019

aabadie commented Feb 13, 2019

cladmi left a comment

cladmi Mar 26, 2019

miri64 Mar 26, 2019

cladmi Mar 26, 2019

miri64 Mar 26, 2019

miri64 commented Mar 26, 2019

miri64 commented Jun 20, 2019

kaspar030 left a comment

miri64 commented Jun 20, 2019

kaspar030 commented Jun 20, 2019

miri64 commented Jun 20, 2019

gnrc: fix several issues found with LLVM's static code analyzer #10350

gnrc: fix several issues found with LLVM's static code analyzer #10350

Conversation

miri64 commented Nov 8, 2018

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cladmi left a 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

Choose a reason for hiding this comment

miri64 commented Nov 8, 2018

miri64 commented Nov 9, 2018

miri64 commented Nov 9, 2018

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Feb 12, 2019

miri64 commented Feb 12, 2019 • edited Loading

aabadie commented Feb 12, 2019

miri64 commented Feb 12, 2019

miri64 commented Feb 12, 2019 • edited Loading

aabadie commented Feb 13, 2019

aabadie commented Feb 13, 2019

cladmi left a 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

miri64 commented Mar 26, 2019

miri64 commented Jun 20, 2019

kaspar030 left a comment

Choose a reason for hiding this comment

miri64 commented Jun 20, 2019

kaspar030 commented Jun 20, 2019

miri64 commented Jun 20, 2019

miri64 commented Feb 12, 2019 •

edited

Loading

miri64 commented Feb 12, 2019 •

edited

Loading