Runtime defined count of Alarms need exposes "Use-after-free in RtcAlarmManager"

[Crypter]

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.

To Reproduce

void setup(){

// constructor is called, then default assignment operator, then destructor - making the _alarms refference point to freed memory.
globalAlarms = RtcAlarmManager<callback>(runtime_defined_count);

}

Expected behavior Modifying source modifies destination.

Deleting source leaves destination with use-after-free of _alarms.

Additional context Continuing the discussion from Pull request #213

[Crypter]

I do not know the number of alarms untill runtime, so I initialize the variable the variable in setup() - hence the need for the assignment operator overload. Other option would be to implement begin() like logic for initilizing the variable.

Same with the alarms being reloaded after user adjustment. There's no point in removing them when I can just assign the variable to a fresh instance.

And there's plenty of use-cases for multiple instances. My use case is that I have N of the same peripheral hardware on one microcontroler, so giving each hardware instance their own manager completely makes sense, as well as storing it with the object that manages it.

While it can be used as a singleton, I realy don't see AlarmManager as one in my design, and this bug prevents me to use it that way. Even the SPI class allows this, despite having a hardware limitation on how many SPI peripherals you can have.

[Makuna]

(I updated the title to be clearer as to what you are asking)

Your line of usage is a problematic c++ issue, where you create and assign. It's a pattern to be avoided with non-atomic types (ones that allocate in constructors for example) since it first allocates then copies, of which the newly copied must allocate internals, and then the original is freed as it is no longer needed. It fragments the heap at best. There is a normal technique to stop consumers from doing this with classes that allocate memory, and I didn't apply that here. I will.

But then this will stop the exact thing you are doing. It will become a compile time error.

Specific dynamic support added Add a Begin(), and in begin the allocations happen. This doesn't solve your pattern of assignment as they will not compile when I fix this. So...

If you want to continue your current use pattern: Use a pointer to the object. You want dynamic allocations (runtime defined); in c++ you should then be thinking "new".

typedef RtcAlarmManager<callback>  AlarmManagerType;
AlarmManagerType* globalAlarms;

...
globalAlarms = new AlarmManagerType(runtime_defined_count);

Assignment will only copy the pointer (reference) to the object, not duplicate it.

just random notes around this topic: While c++ has references, they are only useful for arguments to functions as syntax sugar for pointers; and normally should always be const so assignment to them stops not easily understandable bugs.

Depending on your Ardunio platform type (AVR, ESP32, RP2040), some come with STL and some don't. You can then wrap the AlarmManagerType with a smart pointer, a much-preferred way to handle this, but not all Arduino platforms come with STL so as a library I have to avoid it.

[Makuna]

https://github.com/Makuna/Rtc/pull/215 (redacted) This was a poor solution. See next comment.

[Makuna]

https://github.com/Makuna/Rtc/pull/216 Cleaned up how the AlarmManager manages the callback, allowing lambda to be used for the callback even. See the example. Below is also a demonstration of how to implement a class that contains an AlarmManager instance and has the callback as a member function.

class AlarmGroup
{
public:
  AlarmGroup()
  {
    _alarms.Begin(3);
  }

  void Process()
  {
      _alarms.ProcessAlarms(_alarmCallback, static_cast<void*>(this));
  }

protected:
  static void _alarmCallback(void* context, uint8_t id, const RtcDateTime& alarm)
  {
      AlarmGroup* instance = static_cast<AlarmGroup*>(context);
      instance->OnAlarm(id, alarm);
  }

  void OnAlarm(uint8_t id, const RtcDateTime& alarm)
  {
    // do your work here, within the context of your AlarmGroup
  }

private:
  RtcAlarmManager _alarms;
};

[Makuna]

v2.5.0