Skip to content

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

Closed
wosym wants to merge 1 commit intoRIOT-OS:masterfrom
wosym:lwip_stm32_memfix
Closed

pkg/lwip: fix memory issue when sending with empty sock#12927
wosym wants to merge 1 commit intoRIOT-OS:masterfrom
wosym:lwip_stm32_memfix

Conversation

@wosym
Copy link
Copy Markdown
Member

@wosym wosym commented Dec 11, 2019

Contribution description

Memory-issue described in #12859 fixed by calling extra netconn_delete() on *tmp.

Testing procedure

in tests/lwip: udp send IP_HERE 123 ff 30 500
Before fix: +/- 20 successful sends, memory errors on all later sends
After fix: All sends are successful.

Behaviour has also been tested with ip send. Before fix: memory issues after n successful sends. After fix: infinite successful sends.

Issues/PRs references

Fixes #12859

@benpicco benpicco requested a review from miri64 December 11, 2019 15:31
@benpicco benpicco added Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Dec 11, 2019
@benpicco
Copy link
Copy Markdown
Contributor

CI is not happy.

@wosym wosym force-pushed the lwip_stm32_memfix branch from fe5d0ba to e671b00 Compare December 12, 2019 09:28
return -ENOMEM;
}
if (conn == NULL) {
noconn = 1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could just move this assignment after line 524 and safe an extra check.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mhmmm, but that line does more than only checking of conn == NULL?
if (((conn == NULL) || (*conn == NULL)) && (remote != NULL))
Would you prefer to split this conditional up in multiple parts, and then place the noconn assignment inside the block?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let me have another holistic view on the code...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the code needs a lot more clean-up... Let me open an alternative PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See #12932. This code contained a lot of unnecessary second-level indirections.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 25, 2020

#12932 was merged, so this can be closed.

@miri64 miri64 closed this Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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.

lwip: multiple sock_udp_send() return ENOMEM. (possible memory leak?)

3 participants