Skip to content

gnrc_netif_lorawan: add missing NULL check [backport 2021.01]#16120

Merged
leandrolanzieri merged 1 commit intoRIOT-OS:2021.01-branchfrom
jia200x:backport/2021.01/pr/fix_gnrc_lorawan_pktbuf
Mar 4, 2021
Merged

gnrc_netif_lorawan: add missing NULL check [backport 2021.01]#16120
leandrolanzieri merged 1 commit intoRIOT-OS:2021.01-branchfrom
jia200x:backport/2021.01/pr/fix_gnrc_lorawan_pktbuf

Conversation

@jia200x
Copy link
Member

@jia200x jia200x commented Mar 2, 2021

Backport of #16110

Contribution description

This PR adds a missing NULL check when allocating a gnrc_pktsnip_t on reception. If the pktbuf is full, GNRC netif doesn't return immediately.

This PR fixes that.

Testing procedure

Configure the Network Server to send a downlink to the node and try this patch:

diff --git a/sys/net/gnrc/netif/lorawan/gnrc_netif_lorawan.c b/sys/net/gnrc/netif/lorawan/gnrc_netif_lorawan.c
index c30519220f..a6fb79cb72 100644
--- a/sys/net/gnrc/netif/lorawan/gnrc_netif_lorawan.c
+++ b/sys/net/gnrc/netif/lorawan/gnrc_netif_lorawan.c
@@ -26,7 +26,7 @@
 #include "net/loramac.h"
 #include "net/gnrc/netreg.h"
 
-#define ENABLE_DEBUG 0
+#define ENABLE_DEBUG 1
 #include "debug.h"
 
 static uint8_t _nwkskey[LORAMAC_NWKSKEY_LEN];
@@ -91,7 +91,7 @@ void gnrc_lorawan_mcps_indication(gnrc_lorawan_t *mac, mcps_indication_t *ind)
 {
     (void)mac;
     gnrc_pktsnip_t *pkt = gnrc_pktbuf_add(NULL, ind->data.pkt->iol_base,
-                                          ind->data.pkt->iol_len,
+                                          1024,
                                           GNRC_NETTYPE_LORAWAN);
     if (!pkt) {
         DEBUG("gnrc_lorawan: mcps_indication: couldn't allocate pktbuf\n");

It should print "gnrc_lorawan: mcps_indication: couldn't allocate pktbuf" when a packet is received

Issues/PRs references

None so far

@jia200x jia200x added Area: LoRa Area: LoRa radio support CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Mar 2, 2021
@jia200x jia200x requested a review from leandrolanzieri March 2, 2021 08:58
leandrolanzieri
leandrolanzieri previously approved these changes Mar 2, 2021
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@leandrolanzieri leandrolanzieri added this to the Release 2021.01 milestone Mar 2, 2021
@leandrolanzieri
Copy link
Contributor

@miri64 any idea of why the python-tests action is failing?

@miri64
Copy link
Member

miri64 commented Mar 2, 2021

@miri64 any idea of why the python-tests action is failing?

dd88ead, which was not part of the release, fixed some linting errors, so it seems like #16058 needs to be backported as well. :-/

@miri64
Copy link
Member

miri64 commented Mar 2, 2021

(it happens due to a newer version of pylint which spots some previously false negatives).

@leandrolanzieri
Copy link
Contributor

@miri64 any idea of why the python-tests action is failing?

dd88ead, which was not part of the release, fixed some linting errors, so it seems like #16058 needs to be backported as well. :-/

Thanks!

@leandrolanzieri
Copy link
Contributor

This needs rebasing

@jia200x jia200x force-pushed the backport/2021.01/pr/fix_gnrc_lorawan_pktbuf branch from d4460e2 to f356f06 Compare March 2, 2021 14:53
@jia200x
Copy link
Member Author

jia200x commented Mar 2, 2021

done!

@leandrolanzieri
Copy link
Contributor

The CI is showing a lot of unrelated errors :/

@miri64
Copy link
Member

miri64 commented Mar 3, 2021

Was there a toolchain update in the docker container?

@fjmolinas
Copy link
Contributor

Was there a toolchain update in the docker container?

Yes MIPS toolchain was updated after the release, which could explain the issue RIOT-OS/riotdocker#107

@miri64
Copy link
Member

miri64 commented Mar 3, 2021

Is there a PR that fixed the issues?

@miri64
Copy link
Member

miri64 commented Mar 3, 2021

Versioned docker containers would be great ;-).

@fjmolinas
Copy link
Contributor

Is there a PR that fixed the issues?

Can't find one...

@aabadie
Copy link
Contributor

aabadie commented Mar 3, 2021

I think #16042 should be backported.

@fjmolinas
Copy link
Contributor

@leandrolanzieri can you rebase? CI should pass now.

@leandrolanzieri
Copy link
Contributor

@leandrolanzieri can you rebase? CI should pass now.

@jia200x 's PR :)

@jia200x
Copy link
Member Author

jia200x commented Mar 4, 2021

done!

@jia200x jia200x force-pushed the backport/2021.01/pr/fix_gnrc_lorawan_pktbuf branch from f356f06 to b1f02c9 Compare March 4, 2021 09:43
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-ACK

@leandrolanzieri leandrolanzieri merged commit 69ddaaa into RIOT-OS:2021.01-branch Mar 4, 2021
@jia200x
Copy link
Member Author

jia200x commented Mar 4, 2021

thanks everybody for the review and related backports!!

@miri64
Copy link
Member

miri64 commented Mar 4, 2021

🎉 hardest backport ever ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: LoRa Area: LoRa radio support CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants