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

Michael Balzer dexter at expeedo.de
Tue Oct 4 14:13:38 HKT 2022


Michael,

you're free to implement your debug logging the way you want / need it.

Unless where performance critical or within some ISR context, you can 
decide to keep some logging you'd like to be able to get from users 
later on in the release. Put that on level debug or verbose, so it won't 
spam the log unless enabled for the tag. Btw, you're also free to use 
different tags, e.g. for functional units.

Regards,
Michael


Am 03.10.22 um 09:32 schrieb Michael Geddes:
> I can live with those astyle settings, so I'll include them.
>
> I've been using  vim: tabstop=2 shiftwidth=2 expandtabs  (which is the 
> same as --indent=spaces=2 afaict) - so that's a relief.  Mixed tabs 
> and spaces are a pitb..
>
> While I am used to delphi and so have been using the style that puts 
> the brace at the indent level l'm happy to go with the one you have 
> chosen.
> I really dislike that style with the brace indented. While I still 
> like much about C++, especially the template engine, I do miss delphi 
> enums, sets and a few other features!
>
> I'll download astyle and make it nice before I push up. I've been 
> making lots of little commits as I'm going, but will probably just 
> squash it into one initial.
>
> I'm wondering what I should do about my instrumented function calls!  
> For example - All my functions look like this at the moment (with 
> XARM() and XDISARM)
>
> bool OvmsBatteryMon::checkStateChange()
> {
> XARM("OvmsBatteryMon::checkStateChange");
> if (!m_dirty)
>   {
> XDISARM;
> return false;
>   }
> OvmsBatteryState newState = calc_state();
> bool res = newState != m_lastState;
> m_lastState = newState;
> m_dirty = false;
> XDISARM;
> return res;
> }
>
> These are Macros so they would process  to blank for the release.  The 
> idea is that if the object created by XARM() is not disarmed, it will 
> cause a log to happen. (ie if there's an unexpected exception).  
> Should I just leave them in?
>
> //.ichael
>
>
> On Mon, 3 Oct 2022 at 14:26, Michael Balzer <dexter at expeedo.de> wrote:
>
>     Thanks, Michael.
>
>     > The main problem was the diversity of tab style.
>
>     Yes… I see you even tried to fix indentation style where it seems
>     to have been mixed unintentionally by the module developers ^_^
>
>     Btw, I've begun including EditorConfigs and Artistic Style configs
>     for newer modules, e.g.:
>     -
>     https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/blob/master/vehicle/OVMS.V3/components/zip/.editorconfig
>     -
>     https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/blob/master/vehicle/OVMS.V3/components/vehicle_vweup/src/.astylerc
>
>     Regards,
>     Michael
>
>
>     Am 03.10.22 um 04:07 schrieb 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 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
>>
>>             _______________________________________________
>>             OvmsDev mailing list
>>             OvmsDev at lists.openvehicles.com
>>             http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>>
>>
>>     _______________________________________________
>>     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
>
>     _______________________________________________
>     OvmsDev mailing list
>     OvmsDev at lists.openvehicles.com
>     http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>
>
> _______________________________________________
> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221004/27202094/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 203 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221004/27202094/attachment-0001.sig>


More information about the OvmsDev mailing list