[libs] Fix mDNS string memory corruption, print error on record add failure #260
Conversation
| @@ -3,7 +3,7 @@ | |||
| #include "mDNS.h" | |||
|
|
|||
| static char *ensureUnderscore(const char *value) { | |||
| uint8_t len = strlen(value) + 1; | |||
| uint8_t len = strlen(value) + 1 + 1; // 1 for underscore, 1 for null-terminator | |||
There was a problem hiding this comment.
Should this be conditional on value[0] == '_' (or !=) like below for that extra 1?
Granted malloc'ing 1 extra byte isnt the end of the world, and it's free'd relatively soon after anyway.
There was a problem hiding this comment.
Right, we can save one byte this way.
Anyway, I can see a lot of memory allocations there, maybe some of them could be avoided... Like - when there's no need for underscore, why duplicating the string at all?
There was a problem hiding this comment.
yeah, if (value[0] == '_') return value; seems like the better optimization.
Edit: but then the free in a different function becomes more difficult, doesn't it?
| return result; | ||
| } | ||
|
|
||
| static void freeIfDuplicate(const char *original, const char *duplicate) { |
There was a problem hiding this comment.
A better name could be freeIfCopied - I only realized why it frees if original != duplicate after a few minutes. Somehow "duplicate" makes me think of it as "equal" or "the same".
Also, try making it inline for performance reasons.
There was a problem hiding this comment.
I misspelled, I should've named it duplicated, which is a synonim for copied. But copied is fine too.
- Free allocated memory both on end() and in the destructor - Unregister callback from netif
|
Ok, I think this is enough improvements for now. The only thing left would be to call the new cleanup() method somewhere. In current implementation it will only be called in destructor, but since this is a static object that would be... never? |
|
That's the problem in embedded/IoT programming - these devices most often run as long as they're given power. There's no shutdown, no close button. ESPHome probably never actually calls the shutdown method under normal circumstances, because why would it? |
|
I work in embedded for some time now and I've seen this "we won't need a shutdown" approach many times. And in many cases we eventually did need it :), only by then it was very hard to achieve due to many decisions like not doing proper deregistration or deallocations, using static objects and singletons. And the reasons for needing this in embedded is one: to prevent a full reboot, which usually comes with a price of having to re-initialize and re-register everything, while the only thing you wanted was to reinitialize one small component. The fact that esphome component does have a shutdown method means they are thinking about having this, so I would say it's worth to be prepared :) |
|
Right, but that's a topic for another PR anyway 🙂 |
kuba2k2
changed the title
strcpy() always adds null-terminator at the end.
The original code allocated one byte less than what is needed, so it was overwriting something. Could've been important or not :)
Found this while debugging #248, but I don't know if it fixes it or not. In my local tests on ubuntu I was not able to reproduce the behavior described in #248. But if it's memory related, then it's bound to behave differently on x86 + linux. Worth checking if it helped.