Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WiFiUDP:parsePacket() Crashfix #7847

Merged
merged 2 commits into from
Feb 20, 2023

Conversation

mrengineer7777
Copy link
Collaborator


Description of Change

Resolves crash in WiFiUDP:parsePacket() where allocation of new char[1460]; throws an exception under low memory conditions.

Tests scenarios

Tested on FeatherESP32 using UDP socket listener. Using arduino-esp32 core v2.0.5.
Additional testing welcome!

Related links

Closes #4104 (stale/closed), #7558, #7845

@mrengineer7777
Copy link
Collaborator Author

Notes: I'm not sure how to use nothrow with new keyword. malloc does not throw an exception.

Might need nothrow on new cbuf(len);, as well as checking that the allocation succeeded. Feedback welcome.

@mrengineer7777
Copy link
Collaborator Author

Note to maintainers: Not sure that we need to run checks on draft PRs.

@mrengineer7777
Copy link
Collaborator Author

new cbuf(len); calls constructor for cbuf class. The constructor contains a new char[] call. Can this throw an exception?

cbuf::cbuf(size_t size) :
    next(NULL), _size(size+1), _buf(new char[size+1]), _bufend(_buf + size + 1), _begin(_buf), _end(_begin)
{
}

@mrengineer7777
Copy link
Collaborator Author

Found the solution for nothrow in esp8266/Arduino@6925982

@me-no-dev
Copy link
Member

Hi @mrengineer7777 :) In reality we should transition from using cbuf to something more FreeRTOS related, like Queue (for single bytes) and Ringbuf (for multiple bytes). cbuf should really get phased out and removed in Arduino 3.0.0

@mrengineer7777
Copy link
Collaborator Author

@me-no-dev That is a fantastic idea. For now let's fix the current bug. I will create a new issue (feature request) to remind us to refactor WiFiUDP().

@mrengineer7777 mrengineer7777 marked this pull request as ready for review February 15, 2023 16:22
@VojtechBartoska
Copy link
Contributor

@mrengineer7777 Thanks for the fix and also follow-up with the issue.

@me-no-dev
Copy link
Member

@mrengineer7777 instead of removing cbuf altogether, I decided to rework it to use FreeRTOS Ringbuffer and Semaphore. You can see the progress here. That should improve it for anything that uses cbuf. Your fixes still apply :)

@me-no-dev
Copy link
Member

BTW, what is the status? Does this fix your issue? Can we merge it and have it as part of 2.0.7?

@mrengineer7777
Copy link
Collaborator Author

@me-no-dev I'm excited about the change on cbuf. Looks promising. Will review later.

@me-no-dev This PR fixes the UDP issue for me.

@me-no-dev me-no-dev merged commit 6d64a3b into espressif:master Feb 20, 2023
@mrengineer7777 mrengineer7777 deleted the UDP_ParsePacket_Fix branch February 20, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WiFiUDP::parsePacket() CRASH
4 participants