Speicherverbrauch steigt mit der Zeit

Ah, Mit statischer Codeanalyse kann man auch manchmal schon einen Blumentopf gewinnen. Mit cppcheck habe ich auch schon das eine oder andere gefunden und es gibt noch mehr tools.

kls

Ich versuche es hiermit nochmal einen Beitrag zum finden des Leaks zu leisten:

Mir ist am dem von dir angesprochen Leak Patch aufgefallen, dass hier noch was nicht stimmen könnte.

In der eit.c wird ein Objekt vom Type cSectionSyncerEntry (child object von cListObject und cSectionSyncer und somit größer als cListObject) allokiert, in der tools.c wird aber ein Objekt vom Type cListObject wieder freigegeben. Und das ist 32 Bytes kleiner.

Patch, der das Problem aufzeigt und fixed (sollte jemand das verwenden wollen, bitte erst die printf entfernen, die sind nur zum Aufzeigen des Speichers drin):

diff --git a/eit.c b/eit.c
index ffdec41..91e7d53 100644
--- a/eit.c
+++ b/eit.c
@@ -38,6 +38,7 @@ cEIT::cEIT(cSectionSyncerHash &SectionSyncerHash, int Source, u_char Tid, const
   cSectionSyncerEntry *SectionSyncerEntry = SectionSyncerHash.Get(HashId);
   if (!SectionSyncerEntry) {
      SectionSyncerEntry = new cSectionSyncerEntry;
+     printf("alloc %ld bytes\n", sizeof(*SectionSyncerEntry));  // alloc 72 bytes
      SectionSyncerHash.Add(SectionSyncerEntry, HashId);
      }
   bool Process = SectionSyncerEntry->Sync(getVersionNumber(), getSectionNumber(), getLastSectionNumber());
diff --git a/tools.c b/tools.c
index 7330fce..7328463 100644
--- a/tools.c
+++ b/tools.c
@@ -27,6 +27,7 @@ extern "C" {
 #include <utime.h>
 #include "i18n.h"
 #include "thread.h"
+#include "eit.h"
 
 int SysLogLevel = 3;
 
@@ -2375,8 +2376,13 @@ void cHashBase::Clear(void)
       if (ownObjects) {
          cList<cHashObject> *list = hashTable[i];
          if (list) {
-            for (cHashObject *hob = list->First(); hob; hob = list->Next(hob))
-                delete hob->object;
+            for (cHashObject *hob = list->First(); hob; hob = list->Next(hob)) {
+                printf("size hob %ld\n", sizeof(*hob));  // 40 bytes of cListObject
+                cSectionSyncerEntry *tmp = (cSectionSyncerEntry *) hob->object;  // cast to correct type
+                printf("free %ld bytes \n", sizeof(*tmp));   // size now 72 bytes match alloc size
+                delete tmp;  // free 72 bytes
+//                delete hob->object;  // free 40 bytes
+            }
             }
          }
       delete hashTable[i];

Deine Meinung dazu ?

Zitat von MegaV0lt

Habe jetzt mal cppcheck getestet. Habe einfach mal eine Datei von skinflatplus geckecht (wahllos). Für mich Sinnlos, weil ich mit den Ausgaben nichts anfangen kann:

https://www.dropbox.com/s/1mrmqwe1n1opjs7/cppcheck.txt?dl=0

Da ist jetzt kein detektiertes memory leak dabei aber durchaus ernstzunehmende Hinweise wie nichtinitialisierte Variablen oder der Umgang mit temporären Objekten.

Zitat von kfb77

Deine Meinung dazu ?

Beim ersten printf() in cHashBase::Clear(void) gibst du die Größe von *hob aus, es müsste aber doch wohl die von *hob->object sein, denn das ist es, was freigegeben wird.

Beobachtest du damit dann auch noch eine Diskrepanz?

Ich habe meinen Test-VDR hier jetzt mehrere Tage mit 100ms (statt 1000ms) Wartezeit in cInterface::GetKey() laufen lassen. Damit wird die Hauptschleife zehnmal so oft durchlaufen als normal. Es kam aber zu keiner erkennbaren Steigerung des Speicherverbrauchs. An der Häufigkeit liegt es also wohl nicht.

An Plugins verwende ich übrigens nur ddci2 und vaapidevice. Alle anderen Plugins würde ich zunächst mal bei der Suche nach dem Leck ausschließen.

Mist, copy und paste Fehler aus meinem Test branch.

Ja, der Unterschied ist tatsächlich sogar größer.

diff --git a/eit.c b/eit.c
index ffdec41..91e7d53 100644
--- a/eit.c
+++ b/eit.c
@@ -38,6 +38,7 @@ cEIT::cEIT(cSectionSyncerHash &SectionSyncerHash, int Source, u_char Tid, const
   cSectionSyncerEntry *SectionSyncerEntry = SectionSyncerHash.Get(HashId);
   if (!SectionSyncerEntry) {
      SectionSyncerEntry = new cSectionSyncerEntry;
+     printf("alloc %ld bytes\n", sizeof(*SectionSyncerEntry));  // alloc 72 bytes
      SectionSyncerHash.Add(SectionSyncerEntry, HashId);
      }
   bool Process = SectionSyncerEntry->Sync(getVersionNumber(), getSectionNumber(), getLastSectionNumber());
diff --git a/tools.c b/tools.c
index 7330fce..1993f5e 100644
--- a/tools.c
+++ b/tools.c
@@ -27,6 +27,7 @@ extern "C" {
 #include <utime.h>
 #include "i18n.h"
 #include "thread.h"
+#include "eit.h"
 
 int SysLogLevel = 3;
 
@@ -2375,8 +2376,13 @@ void cHashBase::Clear(void)
       if (ownObjects) {
          cList<cHashObject> *list = hashTable[i];
          if (list) {
-            for (cHashObject *hob = list->First(); hob; hob = list->Next(hob))
-                delete hob->object;
+            for (cHashObject *hob = list->First(); hob; hob = list->Next(hob)) {
+                printf("size hob %ld\n", sizeof(*hob->object));  // 24 bytes of cListObject
+                cSectionSyncerEntry *tmp = (cSectionSyncerEntry *) hob->object;  // cast to correct type
+                printf("free %ld bytes \n", sizeof(*tmp));   // size now 72 bytes match alloc size
+                delete tmp;  // free 72 bytes
+//                delete hob->object;  // free 24 bytes
+            }
             }
          }
       delete hashTable[i];

Das Freigeben von Objekten über den Pointer einer Basisklasse ist eigentlich kein Problem. Die beteiligten Klasse sollte nur einen virtual Destruktor haben.

Die Klasse cListObject hat einen virtual Destruktor. Soll das heißen, sizeof zeigt zwar die Größe der Basisklasse an, delete gibt aber die richtige Größe frei ? Wenn das so ist, wie soll man da Leaks finden ?

Instanzzähler in Klassen?

Dem sizeof den korrekten Typen übergeben?

Sich den Unterschied zwischen C und C++ casts klarmachen (hier lauern gemeine leaks).

Klassen, die abgeleitet werden sollen, müssen einen virtuellen destructor haben (andernfalls leak).

Ressourcenverwaltung klar strukturieren.

shared- bzw. autopointer verwenden.

Immer bewusst sein, welche Methoden der Compiler automatisch generiert (hier lauern gemeine leaks).

Realloc korrekt verwenden (unerwartetes leak im Fehlerfall).

Ohne Anspruch auf Vollständigkeit....

Du könntest beim new und delete den Pointer protokollieren und anschließend die Listen vergleichen. Wenn dann ein Pointer beim delete fehlt, ist es ein Leak. Sowas müsste aber libleak und Konsorten eigentlich finden.

Ich sehe das so: sizeof(*hob->object) liefert die Größe eines cListObject, während sizeof(*tmp) die Größe eines cSectionSyncerEntry liefert. Es ist nicht überraschend, dass die beiden Werte unterschiedlich sind. delete hob->object gibt den Speicher des Objekts an der Stelle frei, und das müsste der Größe von cSectionSyncerEntry entsprechen, denn so viel wurde ja beim Anlegen dieses Objekts allokiert.

Zitat von kls

Ich habe meinen Test-VDR hier jetzt mehrere Tage mit 100ms (statt 1000ms) Wartezeit in cInterface::GetKey() laufen lassen. Damit wird die Hauptschleife zehnmal so oft durchlaufen als normal. Es kam aber zu keiner erkennbaren Steigerung des Speicherverbrauchs. An der Häufigkeit liegt es also wohl nicht.

Damit wäre aber die Hauptschleife inklusive des entsprechenden Threads eigentlich ganz außen vor.

Die Zunahme ist aber weitgehend linear. Fällt dir etwas anderes im VDR ein was mit einer gewissen Regelmäßigkeit läuft?

Sehe ich das richtig das das hier: https://github.com/google/sanitizers/wiki/AddressSanitizer einfach nur einen Compiler-Parameter erfordert?

Bezüglich des Freigebens nach Typwandlung: Speicher wird in aller Regel erstmal nur über seinen Zeiger freigegeben:

https://linux.die.net/man/3/free

Ein "delete" ist auch nicht viel anders als ein "free" das vorher die Destruktoren aufruft.

Unter Umständen kann einem hier natürlich die eine oder andere Eigenart von C++ reinfunken. "Virtuelle Desktruktoren" wurden ja schon genannt.

Bitte auch an korrekte Typwandlung von Zeigern denken! Ich habe oben C-casts gesehen, wo ich dynamic_casts anwenden würde. Ich befürchte, da kann auch etwas schief gehen.

Zitat von kls

Es ist nicht überraschend, dass die beiden Werte unterschiedlich sind

Vielen Dank fürs überprüfen, ich habe wieder was dazu gelernt.

Zitat von M-Reimer

Damit wäre aber die Hauptschleife inklusive des entsprechenden Threads eigentlich ganz außen vor.

Nicht unbedingt, denn egal wie oft die Hauptschleife durchlaufen wird, werden manche Operationen ja nur in bestimmten zeitlichen Abständen durchgeführt (z.B. alles, was mit 'Now' zu tun hat).

Zitat von M-Reimer

Fällt dir etwas anderes im VDR ein was mit einer gewissen Regelmäßigkeit läuft?

EPG-Daten kommen im Mittel mit einer gewissen Rate rein, die relativ gleichmäßig sein dürfte.

Was anderes fällt mir im Moment auch nicht ein.

Müsste nicht bei jeder neuen Aufnahme ein Recording(-Info)-Objekt erstellt werden? Aber das wäre nach einem Neustart immer noch genauso groß wie vorher.

Ich könnte mir auch am ehesten vorstellen, dass irgendwas im Schedules/Events nicht korrekt "vergessen" wird bzw. dort zu viele Objekte vorgehalten werden, die eigentlich nicht mehr gebraucht werden.

Zitat von ReelChris

Mit cppcheck habe ich auch schon das eine oder andere gefunden und es gibt noch mehr tools.

Das tool hatte ich sogar schon installiert.

Ich denke ich hatte es installiert, weil ich es auf den VDR los lassen wollte, hab es dann aber wohl vergessen.

Inzwischen ist das nachgeholt und es hat auch prompt was gefunden:

[ci.c:2885]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour
[libsi/si.c:58]: (error) Invalid number of character '(' when no macros are defined.
[tools.c:1108]: (error) Memory leak: p

Bei den ersten beiden Treffern bin ich mir nicht sicher, ob das wirklich Fehler sind.

Beim Dritten müsste aber was dran sein.

Ich denke, der "strcpy" macht da keinen Sinn:

 --- ./toolsorig.c
+++ ./tools.c
@@ -1103,7 +1103,7 @@
   int l2 = strlen(String);
   char *p = (char *)realloc(s, l1 + l2 + 1);
   if (p != s)
-     strcpy(p, s);
+     s=p;
   strcpy(p + l1, String);
   return *this;
 }

Wobei das so bei einem Rückgabewert von "NULL" noch immer zu Fehlern führt.

Dann hab ich den VDR doch noch zum Laufen gebracht. (Irgendwie ist da Durcheinander mir den Headers passiert.)

Ich hatte den dann jetzt auch ein paar Stunden mit libleak laufen gelassen. Nach ein paar Meldungen ganz am Anfang kam dann aber gar nichts mehr.

Ohne DVB-Input und mit Dummydevice scheint das Speicherleck also nicht aufzutreten.