[Ovmsdev] OvmsMetricString memory lifecycle

[Michael Balzer]

Tom,

in no case can passing & returning string objects by value result in a use after free or memory leak. Pass by value guarantees you'll be operating on a copy as
soon as necessary, and all compiler generated copies are guaranteed to be freed by the compiler.

What will happen on a call by value: a simple compile without any optimization will allocate a new string instance on the heap in substr(), pass that to
SetValue(), let SetValue() create another copy for the assignment, then free the original value and after returning to the calling scope free the heap object.

With optimization the compiler will either directly replace the value by the substr() object, or even execute the compare() on the original strings and directly
let the substr() result write into the existing value object avoiding malloc/free if possible. In both cases, the compiler knows it does not need to free the
temporary / virtual instance because it's become / replaced a persistant instance.

Regards,
Michael


Am 20.08.2018 um 12:36 schrieb Tom Parker:
> I have what I hope is a stupid question.
>
> The additional cellular information code I wrote about yesterday worked for most of a day and then the ovms module stopped communicating with the modem. I
> didn't have a data logger connected so I don't know what exactly happened. When I got on the console the ovms was going through the modem power cycle loop,
> but never receiving data from the modem.
>
> I tried a few things and then I did a simcom status and it hung after printing a few garbage characters instead of the line of information I added to the
> status report yesterday.
>
> This got me to looking at how that memory is handled. I'm doing the following in simcom::StandardLineHandler() which copies the similar pattern for +CGMR and
> +ICCID in the same function:
>
>   else if (line.compare(0, 7, "+CPSI: ") == 0)
>     {
> StandardMetrics.ms_m_net_mdm_cpsi->SetValue(line.substr(7));
>     ESP_LOGI(TAG, "CPSI: %s", line.c_str());
>     }
>
> I know very little about C++ but from what I gather and reading the code
>
> * std:string.substr() creates an object on the stack pointing to memory on the heap
> * OvmsMetricString::SetValue(std::string value) copies the object into it's internal m_value but it still points to the memory allocated on the heap
>
> It's not clear when the heap memory for the std:string created in substr() is freed. I think it is considered for freeing at the end of
> simcom::StandardLineHandler as it goes out of scope. But it escapes the scope when it is passed to OvmsMetricString::SetValue() so if it were freed on
> function exit, we would have a use after free bug.
>
> Is it possible the compiler notices the reference escapes the scope and so doesn't free the string? If it does, then when does it get freed? It surely can't
> be freed when it replaced by the new value inside OvmsMetricString::SetValue() -- at first thought, this pattern without further hints from the programmer
> would make it too easy to end up with double free bugs or memory leaks. If it isn't freed automatically and it isn't freed manually then it looks like we have
> a memory leak.
>
> Looking at the memory logs that the housekeeping message gives, it doesn't look like I have a memory leak, so is there a use after free? If there is, why
> doesn't it cause problems all the time? There are a lot of string metrics and at least 2 that use exactly the same pattern I used.
>
> My changes are at https://github.com/carrott/Open-Vehicle-Monitoring-System-3/tree/cell-info
> _______________________________________________
> OvmsDev mailing list
> OvmsDev at lists.openvehicles.com
> http://lists.openvehicles.com/mailman/listinfo/ovmsdev

-- 
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26