[Ovmsdev] Strange crashes when logging additional info in can logs

[Ludovic LANGE]

Michael,

You're right, that's the best approach ; thanks for sharing it with me.

Here is a PR : 
https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/777 
for review.

(I also converted the second parameter - metric name - but you can 
remove this commit if you prefer)

Regards,

Le 18/11/2022 à 14:51, Michael Balzer a écrit :
> Ludovic,
>
> there is an even better variant of your second approach, that's 
> changing MetricCallbackEntry.m_caller to a std::string.
>
> That's the same scheme EventCallbackEntry already uses, and m_caller 
> is only used to identiy the registrant, so that's straight forward.
>
> Regards,
> Michael
>
>
> Am 18.11.22 um 01:49 schrieb Ludovic LANGE:
>> Some news...
>>
>> My issue was caused because of course I made a mistake with the way 
>> of registering my metrics Listener.
>>
>> I used the following code (see my previous post) in the constructor:
>>
>> + MyMetrics.RegisterListener(IDTAG.c_str(), "*", 
>> std::bind(&canlog::MetricListener, this, _1));
>>
>> and for the destructor:
>> + MyMetrics.DeregisterListener(IDTAG.c_str());
>>
>> I had to use IDTAG because this class has multiple instances.
>>
>> The issue is passing IDTAG.c_str() as a parameter ; 
>> becauseMyMetrics.RegisterListener stores the value of this const 
>> char* parameter and uses it 1) as an immutable parameter and 2) as a 
>> value.
>>
>> Of course this does not work with my first way to call 
>> MyMetrics.RegisterListener :
>>
>>   * the memory buffer returned by IDTAG.c_str() has no more existence
>>     after exiting the constructor, so this lead to memory corruption.
>>   * when calling IDTAG.c_str() again in the destructor, there is
>>     absolutely no chance that this c_str() call returns the same
>>     memory address as the call in the constructor, so the value
>>     comparison will never work.
>>
>>
>> I rewrote the code this way by adding a new member (m_idtag) to the 
>> class:
>>
>> @ vehicle/OVMS.V3/components/can/src/canlog.cpp:352 @ 
>> canlog::canlog(const char* type, std::string format, 
>> canformat::canformat_serve_
>>    m_dropcount = 0;
>>    m_filtercount = 0;
>>
>> +  m_idtag = strdup(IDTAG.c_str());
>> +
>>    using std::placeholders::_1;
>>    using std::placeholders::_2;
>>    MyEvents.RegisterEvent(IDTAG, "*", 
>> std::bind(&canlog::EventListener, this, _1, _2));
>> +  if (NULL != m_idtag)
>> +    {
>> +    MyMetrics.RegisterListener(m_idtag, "*", 
>> std::bind(&canlog::MetricListener, this, _1));
>> +    }
>> +  else
>> +    {
>> +    ESP_LOGE(TAG,"Cannot allocate memory - not registering metrics 
>> listener");
>> +    }
>>
>>    int queuesize = MyConfig.GetParamValueInt("can", "log.queuesize",100);
>>    m_queue = xQueueCreate(queuesize, sizeof(CAN_log_message_t));
>> @ vehicle/OVMS.V3/components/can/src/canlog.cpp:374 @ 
>> canlog::canlog(const char* type, std::string format, 
>> canformat::canformat_serve_
>>  canlog::~canlog()
>>    {
>>    MyEvents.DeregisterEvent(IDTAG);
>> +  if (NULL != m_idtag)
>> +    {
>> +    MyMetrics.DeregisterListener(m_idtag);
>> +    free((void*)m_idtag);
>> +    }
>>
>>    if (m_task)
>>      {
>>
>>
>> Another option is to change MyMetrics.RegisterListener so that it 
>> store *a copy* of the parameter, and does string comparison instead 
>> of a value comparison:
>>
>> modified: vehicle/OVMS.V3/main/ovms_metrics.cpp
>> ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
>> @ vehicle/OVMS.V3/main/ovms_metrics.cpp:1019 @ void 
>> OvmsMetrics::RegisterListener(const char* caller, const char* name, 
>> MetricC
>>      return;
>>      }
>>
>> +  const char * caller_copy = (const char *)strdup(caller);
>> +
>> +  if (NULL == caller_copy)
>> +    {
>> +    ESP_LOGE(TAG, "Problem registering metric %s for caller %s - 
>> insufficient memory",name,caller);
>> +    return;
>> +    }
>> +
>>    MetricCallbackList *ml = k->second;
>> -  ml->push_back(new MetricCallbackEntry(caller,callback));
>> +  ml->push_back(new MetricCallbackEntry(caller_copy,callback));
>>    }
>>
>>  void OvmsMetrics::DeregisterListener(const char* caller)
>> @ vehicle/OVMS.V3/main/ovms_metrics.cpp:1041 @ void 
>> OvmsMetrics::DeregisterListener(const char* caller)
>>      while (itc!=ml->end())
>>        {
>>        MetricCallbackEntry* ec = *itc;
>> -      if (ec->m_caller == caller)
>> +      if (strcmp(ec->m_caller, caller) == 0)
>>          {
>> +        free((char *)ec->m_caller);
>>          itc = ml->erase(itc);
>>          delete ec;
>>          }
>>
>>
>> I've tested both approaches and they both solve my issue : no more 
>> crashes and memory corruption.
>>
>> The second one is more "risky" (as it potentially impacts everyone) 
>> but I think that it should not cause much regression.
>>
>> Let me know what you think - should I make a PR for this second 
>> approach ?
>>
>> Regards,
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20221118/64f2c69d/attachment.htm>