Use of deallocated memory in a bunch of places!?
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
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@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
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@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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
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@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@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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
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... - https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/blob/master... 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@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@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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing list OvmsDev@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
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@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... - https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/blob/master...
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@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@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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing listOvmsDev@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
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@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... - https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/blob/master...
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@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@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@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing list OvmsDev@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev
_______________________________________________ OvmsDev mailing list OvmsDev@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
participants (2)
-
Michael Balzer -
Michael Geddes