Makuna / Rtc

Arduino Library for RTCs, Ds1302, Ds1307, Ds3231, Ds3232, Ds3234 and Pcf8563/BM8563 with deep support. Please refer to the Wiki for more details. Please use the Github Discussions to ask questions as the GitHub Issues feature is used for bug tracking.
GNU Lesser General Public License v3.0
368 stars 126 forks source link

[bugfix] Fix use-after-free memory corruption of RtcAlarmManager objects #213

Closed Crypter closed 1 month ago

Crypter commented 1 month ago

Because this class uses dynamic memory allocation for the _alarms variable, the default copy constructor and assignment operators would cause copy of the reference only, not the data itself. This solves the issue by copying the data on new allocated memory.

Unwanted symptoms by this bug

How to replicate and verify this bug:

RtcAlarmManager<callback> destination = RtcAlarmManager<callback>(1);
// print the value of the _alarms pointer and compare, _alarms needs to be made public
// Serial.printf("%p == %p", destination._alarms, source._alarms);
Makuna commented 1 month ago

While you are pointing out a good bug, your provided solution is not the right direction.

As a "manager" object, it is meant to be a "singleton" and not copied. What would it mean to have two active managers with the same alarms in them? Why would you need it?

Any passing of the object as an argument should be by reference or by pointer as to not copy it. The class should try to enforce this and there are ways.

Further, as a true singleton it shouldn't be passed or stored at all. It just should be accessed from the static single instance. The normal Arduino model of "singleton" is to have a global in your sketch. Another is like String.

If you search up Singleton Pattern you will see the way it should be done.
Please add an "issue" for tracking this.