Bug#33855045 Test ndbinfo_upgrade fails on valgrind builds [#6]

The NdbDictionary::Event returned from
NdbDictionar::Dictionary::getEvent() need to be released.
Fix by:
- Improve function description of NdbDictionary:Dictionary::getEvent()
  to indicate that the returned Event need to be released.
- Add new NdbDictionary:Dictionary::releaseEvent() for releasing the
event returned.
- Add RAII support to NdbApi for managing the lifetime
NdbDictionary::Event using std::unique_ptr, this is enabled when
compiling with at least C++11 support.

This patch fixes potential memory leaks in various places as well as the
one in NdbInfoScanVirtual that was found by valgrind.

Change-Id: Ib52133abff1cd28eef17aeb6cb0527de9fab9574

storage/ndb/include/ndbapi/NdbDictionary.hpp

@@ -25,9 +25,13 @@
 #ifndef NdbDictionary_H
 #define NdbDictionary_H
 
+#if __cplusplus >= 201103L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201103L)
+#include <memory>
+#endif
+
 #include <ndb_types.h>
 
-class Ndb;
+        class Ndb;
 struct CHARSET_INFO;
 
 /* Forward declaration only. */
@@ -2484,12 +2488,20 @@ class NdbDictionary {
     int dropEvent(const char * eventName, int force= 0);
 
     /**
-     * Get event with given name.
+     * Get event instance for given name.
      * @param eventName  Name of event to get.
      * @return an Event if successful, otherwise NULL.
+     *
+     * @note The returned Event need to be released with `releaseEvent`
      */
     const Event * getEvent(const char * eventName);
 
+    /**
+     * Release event previously returned from getEvent()
+     * @param event    The event to release
+     */
+    static void releaseEvent(const Event* event);
+
     /**
      * List defined events
      * @param list   Empty list to hold events returned in the dictionary
@@ -2973,6 +2985,15 @@ class NdbDictionary {
                                     const void* val);
   static
   class NdbOut& printColumnTypeDescription(class NdbOut &, const Column &);
+
+#if __cplusplus >= 201103L  || (defined(_MSVC_LANG) && _MSVC_LANG >= 201103L)
+  // RAII support for the Event returned from Dictionary::getEvent()
+  struct Event_deleter {
+    void operator()(const Event *event) { Dictionary::releaseEvent(event); }
+  };
+  using Event_ptr = std::unique_ptr<const Event, Event_deleter>;
+#endif
+
 
 }; // class NdbDictionary
 

storage/ndb/include/ndbapi/NdbEventOperation.hpp

@@ -337,13 +337,13 @@ class NdbEventOperation {
   friend class NdbEventOperationImpl;
   friend class NdbEventBuffer;
 #endif
-  NdbEventOperation(Ndb *ndb, const NdbDictionary::Event *event);
+  NdbEventOperation(Ndb *ndb, const NdbDictionary::Event* event);
   ~NdbEventOperation();
   class NdbEventOperationImpl &m_impl;
   NdbEventOperation(NdbEventOperationImpl& impl);
 
-  NdbEventOperation(const NdbEventOperation&); // Not impl.
-  NdbEventOperation&operator=(const NdbEventOperation&);
+  NdbEventOperation(const NdbEventOperation&) = delete;
+  NdbEventOperation&operator=(const NdbEventOperation&) = delete;
 };
 
 typedef void (* NdbEventCallback)(NdbEventOperation*, Ndb*, void*);

storage/ndb/plugin/ha_ndbcluster_binlog.cc

@@ -5151,13 +5151,15 @@ int Ndb_binlog_client::create_event(Ndb *ndb,
     }
 
     /*
-      try retrieving the event, if table version/id matches, we will get
+      Try retrieving the event, if table version/id matches, we will get
       a valid event.  Otherwise we have an old event from before
     */
-    const NDBEVENT *ev;
-    if ((ev = dict->getEvent(event_name.c_str()))) {
-      delete ev;
-      return 0;
+    {
+      NdbDictionary::Event_ptr ev(dict->getEvent(event_name.c_str()));
+      if (ev) {
+        // The event already exists in NDB
+        return 0;
+      }
     }
 
     // Old event from before; an error, but try to correct it

storage/ndb/plugin/ndb_binlog_client.cc

@@ -1,5 +1,5 @@
 /*
-   Copyright (c) 2018, 2021, Oracle and/or its affiliates.
+   Copyright (c) 2018, 2022, Oracle and/or its affiliates.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -168,16 +168,12 @@ bool Ndb_binlog_client::event_exists_for_table(Ndb *ndb,
       event_name_for_table(m_dbname, m_tabname, share->get_binlog_full());
 
   // Get event from NDB
-  NdbDictionary::Dictionary *dict = ndb->getDictionary();
-  const NdbDictionary::Event *existing_event =
-      dict->getEvent(event_name.c_str());
+  NdbDictionary::Event_ptr existing_event(
+      ndb->getDictionary()->getEvent(event_name.c_str()));
   if (existing_event) {
     // The event exist
-    delete existing_event;
-
     ndb_log_verbose(1, "Event '%s' for table '%s.%s' already exists",
                     event_name.c_str(), m_dbname, m_tabname);
-
     return true;
   }
   return false;  // Does not exist

storage/ndb/src/ndbapi/NdbBlob.cpp

@@ -245,14 +245,14 @@ NdbBlob::getBlobTable(NdbTableImpl& bt, const NdbTableImpl* t, const NdbColumnIm
 int
 NdbBlob::getBlobEventName(char* bename, Ndb* anNdb, const char* eventName, const char* columnName)
 {
-  NdbEventImpl* e = anNdb->theDictionary->m_impl.getEvent(eventName);
+  std::unique_ptr<NdbEventImpl> e(
+          anNdb->theDictionary->m_impl.getEvent(eventName));
   if (e == NULL)
     return -1;
   NdbColumnImpl* c = e->m_tableImpl->getColumn(columnName);
   if (c == NULL)
     return -1;
-  getBlobEventName(bename, e, c);
-  delete e; // it is from new NdbEventImpl
+  getBlobEventName(bename, e.get(), c);
   return 0;
 }
 

storage/ndb/src/ndbapi/NdbDictionary.cpp

@@ -3283,6 +3283,11 @@ NdbDictionary::Dictionary::getEvent(const char * eventName)
   return 0;
 }
 
+void NdbDictionary::Dictionary::releaseEvent(
+    const NdbDictionary::Event *event) {
+  delete event;
+}
+
 int
 NdbDictionary::Dictionary::listEvents(List& list)
 {

storage/ndb/src/ndbapi/NdbDictionaryImpl.cpp

@@ -6287,17 +6287,15 @@ NdbDictionaryImpl::getEvent(const char * eventName, NdbTableImpl* tab)
   DBUG_ENTER("NdbDictionaryImpl::getEvent");
   DBUG_PRINT("enter",("eventName= %s", eventName));
 
-  NdbEventImpl *ev =  new NdbEventImpl();
+  std::unique_ptr<NdbEventImpl> ev = std::make_unique<NdbEventImpl>();
   if (ev == NULL) {
     DBUG_RETURN(NULL);
   }
 
   ev->setName(eventName);
 
-  int ret = m_receiver.createEvent(*ev, 1 /* getFlag set */);
-
+  const int ret = m_receiver.createEvent(*ev, 1 /* getFlag set */);
   if (ret) {
-    delete ev;
     DBUG_RETURN(NULL);
   }
 
@@ -6309,7 +6307,6 @@ NdbDictionaryImpl::getEvent(const char * eventName, NdbTableImpl* tab)
     if (tab == 0)
     {
       DBUG_PRINT("error",("unable to find table %s", ev->getTableName()));
-      delete ev;
       DBUG_RETURN(NULL);
     }
     if ((tab->m_status != NdbDictionary::Object::Retrieved) ||
@@ -6323,7 +6320,6 @@ NdbDictionaryImpl::getEvent(const char * eventName, NdbTableImpl* tab)
       if (tab == 0)
       {
         DBUG_PRINT("error",("unable to find table %s", ev->getTableName()));
-        delete ev;
         DBUG_RETURN(NULL);
       }
     }
@@ -6349,15 +6345,13 @@ NdbDictionaryImpl::getEvent(const char * eventName, NdbTableImpl* tab)
       table_version_major(ev->m_table_version))
   {
     m_error.code = 241;
-    delete ev;
     DBUG_RETURN(NULL);
   }
 
   if ( attributeList_sz > (uint) table.getNoOfColumns() )
   {
     m_error.code = 241;
     DBUG_PRINT("error",("Invalid version, too many columns"));
-    delete ev;
     DBUG_RETURN(NULL);
   }
 
@@ -6367,7 +6361,6 @@ NdbDictionaryImpl::getEvent(const char * eventName, NdbTableImpl* tab)
     {
       m_error.code = 241;
       DBUG_PRINT("error",("Invalid version, column %d out of range", id));
-      delete ev;
       DBUG_RETURN(NULL);
     }
     if (!mask.get(id))
@@ -6419,7 +6412,6 @@ NdbDictionaryImpl::getEvent(const char * eventName, NdbTableImpl* tab)
         {
           DBUG_PRINT("error", ("Failed to get blob event for column %u",
                                col->getColumnNo()));
-          delete ev;
 
           /**
            * DICT will return error code 723 if the event exists but
@@ -6455,11 +6447,11 @@ NdbDictionaryImpl::getEvent(const char * eventName, NdbTableImpl* tab)
                          blob_count,
                          blob_event_count));
     m_error.code = 241; /* Invalid schema object version */
-    delete ev;
     DBUG_RETURN(NULL);
   }
 
-  DBUG_RETURN(ev);
+  // Return the successfully created event
+  DBUG_RETURN(ev.release());
 }
 
 // ev is main event and has been retrieved previously
@@ -6478,8 +6470,7 @@ NdbDictionaryImpl::getBlobEvent(const NdbEventImpl& ev, uint col_no)
   char bename[MAX_TAB_NAME_SIZE];
   NdbBlob::getBlobEventName(bename, &ev, col);
 
-  NdbEventImpl* blob_ev = getEvent(bename, blob_tab);
-  DBUG_RETURN(blob_ev);
+  DBUG_RETURN(getEvent(bename, blob_tab));
 }
 
 void
@@ -6649,10 +6640,10 @@ NdbDictionaryImpl::dropEvent(const char * eventName, int force)
   DBUG_ENTER("NdbDictionaryImpl::dropEvent");
   DBUG_PRINT("enter", ("name:%s  force: %d", eventName, force));
 
-  NdbEventImpl *evnt = NULL;
+  std::unique_ptr<NdbEventImpl> evnt;
   if (!force)
   {
-    evnt = getEvent(eventName); // allocated
+    evnt.reset(getEvent(eventName));
     if (evnt == NULL)
     {
       if (m_error.code != 723 && // no such table
@@ -6666,12 +6657,10 @@ NdbDictionaryImpl::dropEvent(const char * eventName, int force)
   }
   if (evnt == NULL)
   {
-    evnt = new NdbEventImpl();
+    evnt = std::make_unique<NdbEventImpl>();
     evnt->setName(eventName);
   }
-  int ret = dropEvent(*evnt);
-  delete evnt;  
-  DBUG_RETURN(ret);
+  DBUG_RETURN(dropEvent(*evnt));
 }
 
 int

storage/ndb/src/ndbapi/NdbEventOperation.cpp

@@ -31,10 +31,13 @@
 
 NdbEventOperation::NdbEventOperation(Ndb *ndb,
                                      const NdbDictionary::Event *event)
-  : m_impl(* new NdbEventOperationImpl(*this, ndb, event))
+  : m_impl(* new NdbEventOperationImpl(*this, ndb, std::move(event)))
 {
 }
 
+NdbEventOperation::NdbEventOperation(NdbEventOperationImpl &impl)
+    : m_impl(impl) {}
+
 NdbEventOperation::~NdbEventOperation()
 {
   NdbEventOperationImpl * tmp = &m_impl;
@@ -268,9 +271,6 @@ int NdbEventOperation::getNdbdNodeId() const
  * Private members
  */
 
-NdbEventOperation::NdbEventOperation(NdbEventOperationImpl& impl) 
-  : m_impl(impl) {}
-
 const struct NdbError & 
 NdbEventOperation::getNdbError() const {
   return m_impl.getNdbError();

storage/ndb/src/ndbapi/NdbEventOperationImpl.cpp

@@ -4267,14 +4267,14 @@ NdbEventBuffer::createEventOperation(const char* eventName,
   }
 
   NdbDictionary::Dictionary *dict = m_ndb->getDictionary();
-  const NdbDictionary::Event *event = dict->getEvent(eventName);
+  NdbDictionary::Event_ptr event(dict->getEvent(eventName));
   if (!event)
   {
     theError.code= dict->getNdbError().code;
     return nullptr;
   }
 
-  NdbEventOperation* tOp= new NdbEventOperation(m_ndb, event);
+  NdbEventOperation* tOp= new NdbEventOperation(m_ndb, event.release());
   if (tOp == 0)
   {
     theError.code= 4000;

storage/ndb/src/ndbapi/NdbEventOperationImpl.hpp

@@ -556,7 +556,7 @@ class NdbEventOperationImpl : public NdbEventOperation {
 public:
   NdbEventOperationImpl(NdbEventOperation &f,
                         Ndb *ndb,
-                        const NdbDictionary::Event *myEvnt);
+                        const NdbDictionary::Event* event);
   NdbEventOperationImpl(Ndb *theNdb,
                         NdbEventImpl *evnt);
   void init();

storage/ndb/src/ndbapi/NdbIndexStatImpl.cpp

@@ -2911,19 +2911,17 @@ int
 NdbIndexStatImpl::check_sysevents(Ndb* ndb)
 {
   Sys sys(this, ndb);
-  NdbDictionary::Dictionary* const dic = ndb->getDictionary();
 
   if (check_systables(sys) == -1)
     return -1;
 
-  const char* const evname = NDB_INDEX_STAT_HEAD_EVENT;
-  const NdbDictionary::Event* ev = dic->getEvent(evname);
-  if (ev == 0)
+  NdbDictionary::Event_ptr ev(
+    ndb->getDictionary()->getEvent(NDB_INDEX_STAT_HEAD_EVENT));
+  if (ev == nullptr)
   {
-    setError(dic->getNdbError().code, __LINE__);
+    setError(ndb->getDictionary()->getNdbError().code, __LINE__);
     return -1;
   }
-  delete ev; // getEvent() creates new instance
   return 0;
 }
 

storage/ndb/src/ndbapi/NdbInfoScanVirtual.cpp

@@ -1596,7 +1596,7 @@ class EventsTable : public VirtualTable {
       const override {
 
     const DictionaryList::Element * elem;
-    std::unique_ptr<const NdbDictionary::Event> event;
+    NdbDictionary::Event_ptr event;
 
     do {
       elem = ctx->nextInList(row);

storage/ndb/test/ndbapi/test_event.cpp

@@ -1,5 +1,5 @@
 /*
- Copyright (c) 2003, 2021, Oracle and/or its affiliates.
+ Copyright (c) 2003, 2022, Oracle and/or its affiliates.
 
  This program is free software; you can redistribute it and/or modify
  it under the terms of the GNU General Public License, version 2.0,
@@ -4041,7 +4041,7 @@ int runTryGetEvent(NDBT_Context* ctx, NDBT_Step* step)
   {
     g_err << "Attempting to get the event, expect "
           << ((odd?"success":"failure")) << endl;
-    const NdbDictionary::Event* ev = myDict->getEvent(eventName);
+    NdbDictionary::Event_ptr ev(myDict->getEvent(eventName));
 
     if (odd)
     {