[Ovmsdev] Use of deallocated memory in a bunch of places!?

[Michael Geddes]

Pull request created.

I hope it's ok.
The main problem was the diversity of tab style.

//.ichael

On Mon, 3 Oct 2022 at 09:23, Michael Geddes <frog at bunyip.wheelycreek.net>
wrote:

> Not 8 issues.  8 Files that I found issues in, each with a bunch of them -
> so around 50 all up.
>
> I did a vimgrep for lines starting with something like 'const char* ident
> =' ish...
>
> Yeah, I agree, checking for IsDefined() is a lot less cludgy, so have done
> that (yay for vim macros). Also the existing code didn't cope with -ve
> values!
>
> I guess I better fork the project now and submit a pull request!! :)
> //.ichael
>
> On Sun, 2 Oct 2022 at 22:01, Michael Balzer <dexter at expeedo.de> wrote:
>
>> Michael,
>>
>> oh, the dreadful c_str() trap once again… nice find.
>>
>> The deallocated heap buffer *can* have been reallocated easily, we're
>> running on a preemptive OS and on two CPU cores.
>>
>> IOW, all of these need to be fixed. Sure there are only 8 of them?
>>
>> On your is_empty_metric() utility: may be I miss something, but why don't
>> we simply use the metric's IsDefined() method?
>>
>> Regards,
>> Michael
>>
>>
>> Am 02.10.22 um 13:37 schrieb Michael Geddes:
>>
>> Hi all,
>>
>> I noticed some behavior on the *tpms* command I'd copied from the
>> Kia/Kona code, which sent me down a small rabbit hole.
>> What I was seeing was that the temperature was repeated instead of seeing
>> the pressure and temperature.
>>
>> Even in vehicle.cpp, we can see code like this:
>>
>>   const char* range_est =
>> StdMetrics.ms_v_bat_range_est->AsUnitString("-", rangeUnit, 0).c_str();
>>   if (*range_est != '-')
>>     writer->printf("Est. range: %s\n", range_est);
>>
>> My C++ is a bit rusty, however I believe that the 'const char*' won't
>> hold the temporary std::string object returned by 'AsUnitString'.  To do
>> that, I believe we need to assign to a reference (which I know will work).
>> The problem is that the original code will mostly work (especially in cases
>> like the above), as the deallocated heap memory won't have been reallocated.
>>
>>   const std::string& range_est =
>> StdMetrics.ms_v_bat_range_est->AsUnitString("-", rangeUnit, 0);
>>   if (!is_empty_metric(range_est))
>>     writer->printf("Est. range: %s\n", range_est.c_str());
>>
>> Where I've defined in vehicle.h the following:
>> inline bool is_empty_metric(const std::string &measure)
>> {
>>   return (measure == "") || (measure[0] == '-');
>> }
>>
>> Will the above work functionally?  Would it be better to be:
>>
>> return (measure =="") || (measure == "-")
>>
>> This occurs in 8 files (outside my new Ioniq 5 files) that I have noted.
>>
>> I would make a separate commit for this and do a pull request for it,
>>
>> //.ichael
>>
>>
>> _______________________________________________
>> OvmsDev mailing listOvmsDev at lists.openvehicles.comhttp://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>
>>
>> --
>> Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
>> Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
>>
>> _______________________________________________
>> OvmsDev mailing list
>> OvmsDev at lists.openvehicles.com
>> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221003/2a470758/attachment.htm>