search

libretiny-eu / libretiny

PlatformIO development platform for IoT modules
http://docs.libretiny.eu/
MIT License
382 stars 55 forks source link

[libs] Fix mDNS string memory corruption, print error on record add failure #260

Closed szupi-ipuzs closed 3 months ago

szupi-ipuzs commented 4 months ago

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.

szupi-ipuzs commented 4 months ago

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? Shouldn't the mDNS.end() be called in MDNSComponent::on_shutdown() (in mdns_libretiny.cpp)? That would call cleanup right about when it should be, right? Currently it isn't called at all if I'm not mistaken.

kuba2k2 commented 4 months ago

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? The point is, with the current Arduino API, there's only so much you can improve. I'd say it's okay the way it is and that there's no need to worry about this.

szupi-ipuzs commented 3 months ago

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 :)

kuba2k2 commented 3 months ago

Right, but that's a topic for another PR anyway :slightly_smiling_face: