Leak in POST /v2/op/notify operation (not critical)

[fgalan]

A leak has been found in the POST /v2/op/notify operation. To raise the problem:

• Uncomment the following line in

#orionCurl --url /v2/op/notify -X POST   --payload "$payload"  > /dev/null

• Run:

./valgrindTestSuite.sh statistics_with_counters.test

you'll get:

Test 001/1: 0000_statistics_operation/statistics_with_counters ...................................................................................... FAILED (lost: 632). Check statistics_with_counters.valgrind.out for clues

In the log we can see:

==1573955== 632 (320 direct, 312 indirect) bytes in 1 blocks are definitely lost in loss record 35 of 35
==1573955==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
==1573955==    by 0x3352F6: parseNotificationData(ConnectionInfo*, rapidjson::GenericMemberIterator<true, rapidjson::UTF8<char>, rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >, NotifyContextRequest*, OrionError*) (parseNotification.cpp:113)
==1573955==    by 0x336469: parseNotificationNormalized(ConnectionInfo*, NotifyContextRequest*, OrionError*) (parseNotification.cpp:213)
==1573955==    by 0x336F4E: parseNotification[abi:cxx11](ConnectionInfo*, NotifyContextRequest*) (parseNotification.cpp:250)
==1573955==    by 0x312EBB: jsonRequestTreat(ConnectionInfo*, ParseData*, RequestType, JsonDelayedRelease*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (jsonRequestTreat.cpp:168)
==1573955==    by 0x2CA892: payloadParse(ConnectionInfo*, ParseData*, RestService*, JsonRequest**, JsonDelayedRelease*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (RestService.cpp:199)
==1573955==    by 0x2CCA6B: restService(ConnectionInfo*, RestService*) (RestService.cpp:622)
==1573955==    by 0x2CDD2B: orion::requestServe[abi:cxx11](ConnectionInfo*) (RestService.cpp:774)
==1573955==    by 0x2C6CF8: connectionTreat(void*, MHD_Connection*, char const*, char const*, char const*, char const*, unsigned long*, void**) (rest.cpp:1637)
==1573955==    by 0x443C4E: call_connection_handler (connection.c:2220)
==1573955==    by 0x4455B7: MHD_connection_handle_idle (connection.c:3513)
==1573955==    by 0x4472DD: call_handlers (daemon.c:1215)

The relevant code in parseNotification.cpp is:

  for (rapidjson::Value::ConstValueIterator iter2 = iter->value.Begin(); iter2 != iter->value.End(); ++iter2)
  {
    ContextElementResponse* cerP = new ContextElementResponse();   <------ THIS IS THE NEW() THAT IS LEAKING

    ncrP->contextElementResponseVector.vec.push_back(cerP);

    if (parseContextElementResponse(ciP, iter2, cerP, oeP) == false)
    {
      return false;
    }
  }

It is not critical given that, as far as I know, nobody is actually using the POST /v2/op/notify operation.

[mapedraza]

Based on other parts of the code, something like this should fix it:

  for (rapidjson::Value::ConstValueIterator iter2 = iter->value.Begin(); iter2 != iter->value.End(); ++iter2)
  {
    ContextElementResponse* cerP = new ContextElementResponse();

    ncrP->contextElementResponseVector.vec.push_back(cerP);

    if (parseContextElementResponse(ciP, iter2, cerP, oeP) == false)
    {
      ncrP->contextElementResponseVector.release();
      return false;
    }
  }
  ncrP->contextElementResponseVector.release();
  return true;
}

[fgalan]

Based on other parts of the code, something like this should fix it:

  for (rapidjson::Value::ConstValueIterator iter2 = iter->value.Begin(); iter2 != iter->value.End(); ++iter2)
  {
    ContextElementResponse* cerP = new ContextElementResponse();

    ncrP->contextElementResponseVector.vec.push_back(cerP);

    if (parseContextElementResponse(ciP, iter2, cerP, oeP) == false)
    {
      ncrP->contextElementResponseVector.release();
      return false;
    }
  }
  ncrP->contextElementResponseVector.release();
  return true;
}

I'm afraid is not a valid solution...

Considering the snipped in its context https://github.com/telefonicaid/fiware-orion/blob/master/src/lib/jsonParseV2/parseNotification.cpp#L113, note that the purpose is to fill the ncrP vector to be used by the caller of parseNotificationData() so we cannot release just after parsing.

[PradyumnAgrawal05]

Hi,@fgalan.

I've gone through this issue #4261, According to the description, the new operator allocates memory that should be eventually freed to avoid memory leaks. I propose the introduction of std::unique_ptr to oversee the memory allocation and deallocation for ContextElementResponse objects in theparseNotificationData function. The smart pointer ensures automatic memory deallocation when the object goes out of scope, ensuring proper memory management and avoiding potential memory leaks. Please provide your opinion and confirm my understanding.

Looking forward to your guidance on this!

[PradyumnAgrawal05]

Hi @fgalan, I proposed the below code modifications and understanding. Please confirm my understanding. Thanks

1. The new operator allocates memory that should eventually be freed to avoid memory leaks.
2. One approach is to use smart pointers, such as std::unique_ptr, which automatically handle memory deallocation when the object goes out of scope.
3. In this modification, std:unique_ptr is used to manage the memory allocated for ContextElementResponse.
4. The get () method is used to obtain the raw pointer for adding it to the vector.
5. When the std:unique_ptr cerP goes out of scope, it will automatically release the allocated memory, preventing memory leaks.

Proposed Modifications:-

// Modified Code

for (rapidjson::Value::ConstValueIterator iter2 = iter->value.Begin(); iter2 != iter->value.End(); ++iter2)
{
  std::unique_ptr<ContextElementResponse> cerP(new ContextElementResponse());

  ncrP->contextElementResponseVector.vec.push_back(cerP.get());

  if (parseContextElementResponse(ciP, iter2, cerP.get(), oeP) == false)
  {
    return false;
  }
  // The unique_ptr cerP will automatically release memory when it goes out of scope
}

Looking forward to your guidance on this!

[fgalan]

Thank you for you feedback!

The usage of std::unique_ptr could be a good idea, but it should be applied to the whole codebase, which is a big task that goes out of the scope of this issue.

Using std::unique_ptr in just one place would introduce a lack of homogeneity in the code. It seems that the "unbalanced" new is known (check above), which use to be the hardest part when debugging memory leaks. So now it is just a matter of adding the right delete in the proper place.

[fgalan]

Related: https://stackoverflow.com/questions/78651877/excessive-memory-consumption-in-fiware-orion-when-receiving-a-large-volume-of-no/78692170#78692170