lwip_sock: clean-up connection handling without a sock. by miri64 · Pull Request #12932 · RIOT-OS/RIOT

miri64 · 2019-12-12T10:28:25Z

Contribution description

This is an enhanced version of #12927. In addition to the fix there, it removes the unnecessary additional indirection of conn as either the sock is provided with sock_*_send() or not. In the first case the indirection is not necessary, and in the second we need to delete the created conn within lwip_sock_send() anyway (as introduced in #12927, so returning it makes no sense.

It also adds a check for the call to lwip_sock_send() by the respective protocol callers, so that sock is not dereferenced when it is NULL

Testing procedure

lwip_sock tests should still compile and run for both IPv4 and IPv6 and both network layer protocols combined:

for prot in ip udp tcp; do
    LWIP_IPV4=1 make -C tests/lwip_sock_${prot}/ flash test
    LWIP_IPV6=1 make -C tests/lwip_sock_${prot} flash test
    LWIP_IPV4=1 LWIP_IPV6=1 make -C tests/lwip_sock_${prot} flash test
done

Either the sock is provided with `sock_*_send()` or not. In the first

case the indirection is not necessary, and in the second we need to
delete the created conn within lwip_sock_send() anyway, so returning
it makes no sense.

Issues/PRs references

Contains a modified version of #12927.
Fixes #12859.

Either the sock is provided with `sock_*_send()` or not. In the first case the indirection is not necessary, and in the second we need to delete the created `conn` within `lwip_sock_send()` anyway, so returning it makes no sense.

miri64 · 2019-12-12T10:31:41Z

Just noticed a simpler check for when to delete the netconn. Let me know what you think.

wosym · 2019-12-12T10:59:48Z

I agree that these changes make it a lot cleaner and easier to grasp.
However, you now always delete the conn. Won't this break functions such as udv_recv(), because they rely on this open connection?

miri64 · 2019-12-12T11:07:13Z

Oops, that wasn't my attention... The problem with last minute changes ;-).

wosym · 2019-12-12T11:16:50Z

Looks good now! Clever way of using the tmp as a universal connection, and later on using the conn again to check if tmp needs to be freed or not, removing the need for an extra flag (like I did in my PR).

benpicco · 2019-12-13T09:30:09Z

@wosym does this work for you?

wosym · 2019-12-13T11:49:48Z

@benpicco It seems to work, yes.

tests/lwip:
- udp send --> able to send over 1000 messages without memory issues
- ip send --> able to send over 1000 messages without memory issues
tests/lwip_sock_ip --> all tests succeed
tests/lwip_sock_udp --> all tests succeed

benpicco

Code looks good too.
@miri64 go ahead and squash.

miri64 · 2019-12-13T16:37:58Z

Squashed from a hotel room with blocked SSH access...

miri64 · 2019-12-13T16:40:16Z

Mhhh let's try this again.

miri64 · 2019-12-13T16:43:36Z

Can somebody confirm that worked? Git tells me the branch is up-to-date, but I don't see the squash commits in the GitHub interface...

miri64 · 2019-12-13T16:45:35Z

$ git remote -v
origin	git@github.com:miri64/RIOT.git (fetch)
origin	git@github.com:miri64/RIOT.git (push)
upstream	git://github.com/RIOT-OS/RIOT.git (fetch)
upstream	git://github.com/RIOT-OS/RIOT.git (push)
git show --decorate
commit 49a8f64c62b933d9936675ee56acc71e889a0249 (HEAD -> lwip_sock/fix/sock-cleanup, origin/lwip_sock/fix/sock-cleanup)
Author: Wouter Symons <wsymons@nalys-group.com>
Date:   Wed Dec 11 16:14:52 2019 +0100

    pkg/lwip: fix memory issue when sending with empty sock

diff --git a/pkg/lwip/contrib/sock/lwip_sock.c b/pkg/lwip/contrib/sock/lwip_sock.c
index f236c078f..de38335a2 100644
--- a/pkg/lwip/contrib/sock/lwip_sock.c
+++ b/pkg/lwip/contrib/sock/lwip_sock.c
@@ -564,6 +564,9 @@ ssize_t lwip_sock_send(struct netconn *conn, const void *data, size_t len,
             break;
     }
     netbuf_delete(buf);
+    if (conn == NULL) {
+        netconn_delete(tmp);
+    }
     return res;
 }
$ git show --decorate
commit 49a8f64c62b933d9936675ee56acc71e889a0249 (HEAD -> lwip_sock/fix/sock-cleanup, origin/lwip_sock/fix/sock-cleanup)
Author: Wouter Symons <wsymons@nalys-group.com>
Date:   Wed Dec 11 16:14:52 2019 +0100

    pkg/lwip: fix memory issue when sending with empty sock

diff --git a/pkg/lwip/contrib/sock/lwip_sock.c b/pkg/lwip/contrib/sock/lwip_sock.c
index f236c078f..de38335a2 100644
--- a/pkg/lwip/contrib/sock/lwip_sock.c
+++ b/pkg/lwip/contrib/sock/lwip_sock.c
@@ -564,6 +564,9 @@ ssize_t lwip_sock_send(struct netconn *conn, const void *data, size_t len,
             break;
     }
     netbuf_delete(buf);
+    if (conn == NULL) {
+        netconn_delete(tmp);
+    }
     return res;
 }

miri64 · 2019-12-13T16:57:51Z

The branch even points to the correct commit... crappy hotel wifi is crappy or something is seriously wrong with GitHub...

miri64 added 2 commits December 12, 2019 11:01

lwip_sock: check if sock is set before dereferencing it

80c23cf

lwip_sock: additional indirection of conn is unnecessary

e96bc2a

Either the sock is provided with `sock_*_send()` or not. In the first case the indirection is not necessary, and in the second we need to delete the created `conn` within `lwip_sock_send()` anyway, so returning it makes no sense.

miri64 added this to the Release 2020.01 milestone Dec 12, 2019

miri64 requested a review from benpicco December 12, 2019 10:28

miri64 mentioned this pull request Dec 12, 2019

pkg/lwip: fix memory issue when sending with empty sock #12927

Closed

benpicco added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 13, 2019

benpicco approved these changes Dec 13, 2019

View reviewed changes

pkg/lwip: fix memory issue when sending with empty sock

49a8f64

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 15, 2019

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 9, 2020

miri64 merged commit 87d00ab into RIOT-OS:master Jan 9, 2020

miri64 deleted the lwip_sock/fix/sock-cleanup branch January 9, 2020 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lwip_sock: clean-up connection handling without a sock.#12932

lwip_sock: clean-up connection handling without a sock.#12932
miri64 merged 3 commits intoRIOT-OS:masterfrom
miri64:lwip_sock/fix/sock-cleanup

miri64 commented Dec 12, 2019

Uh oh!

miri64 commented Dec 12, 2019

Uh oh!

wosym commented Dec 12, 2019

Uh oh!

miri64 commented Dec 12, 2019

Uh oh!

wosym commented Dec 12, 2019

Uh oh!

benpicco commented Dec 13, 2019

Uh oh!

wosym commented Dec 13, 2019

Uh oh!

benpicco left a comment

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

miri64 commented Dec 12, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Dec 12, 2019

Uh oh!

wosym commented Dec 12, 2019

Uh oh!

miri64 commented Dec 12, 2019

Uh oh!

wosym commented Dec 12, 2019

Uh oh!

benpicco commented Dec 13, 2019

Uh oh!

wosym commented Dec 13, 2019

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

miri64 commented Dec 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants