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

DNSServer cleanup #5179

Closed
Makuna opened this issue Sep 28, 2018 · 5 comments · Fixed by #5183
Closed

DNSServer cleanup #5179

Makuna opened this issue Sep 28, 2018 · 5 comments · Fixed by #5183
Assignees
Milestone

Comments

@Makuna
Copy link
Collaborator

Makuna commented Sep 28, 2018

Basic Infos

DNSServer does not have a destructor which can cause memory leaks if dynamically allocated.

Problem Description

DNSServer needs a destructor. Currently you can destruct the object and leave the internal _buffer memory still allocated causing a memory leak.

This can be worked around by calling stop() before destructing, but this is an unsafe assumption.

p.s. This library also lacks the keywords file so there is no pretty colors in the Arduino IDE for the methods.

@earlephilhower
Copy link
Collaborator

I'm not seeing any leak potential.

_buffer is only malloc()'d in DNSServer::processNextRequest()

_buffer = (unsigned char*)malloc(_currentPacketSize * sizeof(char));

It's free()'d inside the same if {...} statement before any potential return from the method:

If you've got the time, PRs for things like keywords.txt are always appreciated, too.

@Makuna
Copy link
Collaborator Author

Makuna commented Sep 28, 2018

Wow, I looked deeper, and you are correct. The code in the stop() should never be needed then and should be removed as it implies a use that is not present.

Having the destructor call stop would be still be a good thing.

Further, there should not be a member at all for the _buffer, since it has a limited lifetime, it should just be passed into the methods that will need it rather than store an extra pointer for the lifetime of the object to just avoid passing an argument.

@Makuna Makuna self-assigned this Sep 28, 2018
@Makuna Makuna changed the title DNSServer can leak memory if dynamically allocated/free'ed DNSServer cleanup Sep 28, 2018
@devyte
Copy link
Collaborator

devyte commented Sep 28, 2018

@Makuna there's also my ESPAsyncDNSServer in case you're interested. I only mention it because I've received little feedback, andand I'd like some :p

@devyte
Copy link
Collaborator

devyte commented Sep 30, 2018

@Makuna you self-assigned this. Do you intend to get it done within the next few days?

@Makuna
Copy link
Collaborator Author

Makuna commented Sep 30, 2018

#5194

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 a pull request may close this issue.

3 participants