Kann mal bitte jemand auflisten welche Patches aktuell sind? Dann würde ich die gegen Abend in einem neuen Branch mal einbauen.
Compile Warnings in vnsiserver
-
-
SHF: Genau die gleiche Idee. Die macros machen auch nicht viel anderes.
Wollen wir mit deinem oder meinem Patch weitermachen?
Mir egal, Hauptsache gleicher Code und es ist absolut sicher, dass der Lock aus dem Spiel ist.
@Zeilennummern, ich nutze einen online c++ demangler zur Lesbarkeit und hangele mich dann Zeile für Zeile durch den code, beginnend ab der ersten im Core. Setzt voraus, dass man *genau* den code hat, vdr und plugin.
-
Vielleicht sind es cString::sprintf() und cString::vsprintf(), die nicht threadsafe sind.
Zu vasprintf() gibt es Hinweise, dass die Funktion nur dann threadsafe ist, wenn sich die locale nie ändert.
-
VDR setzt die c-locale am Anfang. 'setlocale(LC_ALL, "")'.
VDR ändert die dann nicht mehr.
Ihr könnt ja mal mit grep nach setlocale im vnsiserver suchen.
~ Markus
P.S.: Ich denke, die c++ locale ist hier irrelevant (?)
-
Genau die gleiche Idee. Die macros machen auch nicht viel anderes.
Inzwischen habe ich da etwas mehr Überblick und im Endeffekt sollten alle 3 Varianten in diesem Fall aufs selbe hinaus laufen.
Wollen wir mit deinem oder meinem Patch weitermachen?
Mir egal, Hauptsache gleicher Code und es ist absolut sicher, dass der Lock aus dem Spiel ist.
Mir auch völlig egal.
Ich hatte es eigentlich nur angehängt, da ich es eh rum fliegen hatte.
Soll doch der Maintainer entscheiden, was er für besser zu pflegen hält.
Inzwischen bin ich aber relativ sicher, dass der Lock nicht die Ursache für den Crash ist, sonder den ganzen Prozess nur in Gang setzt.
Bislang hatte ich vermutet, dass der Neustart das Einlesen der Kanalliste auslöst, das scheint aber nicht so zu sein.
Das zwei Prozesse gleichzeitig einen Readlock auf die Kanalliste bekommen, müsste prinzipiell auch funktionieren. Und die Formulierungen deuten für mich auch darauf hin, dass mehrere Prozesse gleichzeitig einen Readlock auf eine Liste haben können.
Der Ablauf müsste folgender sein:
Der VDR ändert die PID eines Kanals und gibt die Kanalliste wieder frei. Dabei wird der Statekey auch korrekt inkrementiert.
Das merkt jetzt das vnsiplugin und gibt das an die KODIClients weiter. Der Code dazu steckt in status.c, die ganze Datei dient nur dazu die Listen des VDR auf Änderungen zu überwachen.
Die Clients holen sich dann jeweils die neue Kanalliste vom Plugin. Und das startet dann processCHANNELS_GetChannels, ehe die Daten an die Clients geschickt werden.
Einen Einfluss des Neustarts kann ich momentan zwar nicht 100% ausschließen, aber da das Problem auch zur Laufzeit auftritt, lasse ich das hier mal gut sein.
Zum Zeitpunkt der Crashs laufen eigentlich auch nur die beiden for-Schleifen in processCHANNELS_GetChannels. Praktisch alle anderen Prozesse stehen bei einem "wait" oder "poll" das heisst, die tun eigentlich nichts und warten nur.
Der Fehler müsste IMHO also in irgend einem Code-Schnipsel liegen, der in der for-Schleife ausgeführt wird.
crash um 23:19:27
Da hat es mal nicht den Prozess, der in channel->Name() steckt erwischt. (Womit sich meine Frage nach der Zeilennummer erübrigt.)
Vielleicht sind es cString::sprintf() und cString::vsprintf(), die nicht threadsafe sind.
Das will ich mal nicht hoffen, das würde eine Katastrophe nahe kommen
.Aber evtl. ein Bug in der GLIBC?
@Zeilennummern
Danke für den Tipp, das schaue ich mir mal an.
-
Ihr könnt ja mal mit grep nach setlocale im vnsiserver suchen.
Weder setlocale noch locale gibt es.
Vielleicht sind es cString::sprintf() und cString::vsprintf(), die nicht threadsafe sind.
Nach einigem nachdenken habe ich aber keinen kritischen Fall gesehen (was aber nicht heissen muss).
Auf der Kanalliste ist der Readlock, die sollte sich nicht ändern, solange diese for-Schleife läuft.
Und sonst wird nichts mit anderen Threads ausgetauscht - zumindest nicht offensichtlich.
Ich habe aber mal grob überschlagen, dass bei einem Durchlauf der for-Schleife cString::sprintf() oder ein Äquivalent zwischen 10 und 20 Mal aufgerufen werden. Praktisch jeder ausgeführte Befehl mach mindestens einen Aufruf, manche gleich mehrere.
Es könnte auch gut eine Frage der Statistik sein, dass es meist cString::sprintf() trifft.
Dann verwenden noch die Funktionen von cResponsePacket dynamische Speicherverwaltung. Eine üble Rechnerei mit Puffergrösse, verbrauchtem Puffer usw.
Gesehen hab ich zwar nichts auffälliges, aber würde mich nicht wundern, wenn man sich da irgendwo +-1 verrechnet hat.
Da muss ich morgen noch mal ausgeschlafen genauer schauen
.Einiges, was in der Schleife gemacht wird ist IMHO für den Betrieb nicht lebensnotwendig.
Wenn man Lust hat, könnte man mal versuchen möglichst viel zu deaktivieren, um die Suche einzugrenzen:
C++: vnsiclient.c
Alles anzeigen// if (radio != cVNSIChannelFilter::IsRadio(channel)) // continue; //filtert Radiokanäle aus der Liste raus, eigentlich nicht unbedingt nötig // skip invalid channels // if (channel->Sid() == 0) // continue; // notwendig? gibt evtl. Probleme ohne. // if (endswith(channel->Name(), "OBSOLETE")) // continue; //filtert Kanäle mit "OBSOLETE" am Ende raus, sollte auch so gehen // check filter // if (filter && !VNSIChannelFilter.PassFilter(*channel)) // continue; //Favoritenliste, schätze ich. Müsste auch ohne gehen. //den nächsten Block braucht man, sonst kommt nichts bei den Clients an ;-) uint32_t uuid = CreateChannelUID(channel); resp.add_U32(channel->Number()); resp.add_String(m_toUTF8.Convert(channel->Name())); resp.add_String(m_toUTF8.Convert(channel->Provider())); resp.add_U32(uuid); resp.add_U32(channel->Ca(0)); caid_idx = 0; caids = "caids:"; while((caid = channel->Ca(caid_idx)) != 0) { caids = cString::sprintf("%s%d;", (const char*)caids, caid); caid_idx++; } resp.add_String((const char*)caids); if (m_protocolVersion >= 6) { resp.add_String(CreatePiconRef(channel)); } // create entry in EPG map on first query // m_epgUpdate.insert(std::make_pair(uuid, sEpgUpdate())); // Ist wohl nötig, damit man EPG zu dem Kanal bekommt. Geht auch erstmal ohne.Wahrscheinlich wird es bei einem oder mehreren der 4 makierten Blöcke irgendwelche Probleme geben.
Da muss man Probieren, was unbedingt gebraucht wird und was nicht.
-
Hi,
Bin kein Programmierer, aber versucht doch einfach mal statt readlock zum Ursache finden die Channels zu duplizieren so dass jeder Thread seine eigene Kopie hat. Dann kann es ja keine doppelten Zugriffe mehr geben. Ist nur zum Fehler ausschließen, nicht für später.
Nur als Idee.
MfG Stefan
-
Dass bei dir noch die Warnung auftritt, aber bei den anderen nicht, habe ich nicht verstanden.
Aber die Warnung ist nur ein weiteres Problem.
Und es knallt wieder in cString::sprintf() bei vasprintf
-
#1 0x00007f9fe0041176 __GI_raise (libc.so.6 + 0x41176)
#2 0x00007f9fe0028917 __GI_abort (libc.so.6 + 0x28917)
heißt aber AFAIK, dass das geplant aborted wurde, also dass ggf. das kaputt ist, was in den vasprintf gesteckt wurde.
-
#1 0x00007f9fe0041176 __GI_raise (libc.so.6 + 0x41176)
#2 0x00007f9fe0028917 __GI_abort (libc.so.6 + 0x28917)
heißt aber AFAIK, dass das geplant aborted wurde, also dass ggf. das kaputt ist, was in den vasprintf gesteckt wurde.
in vasprintf wird malloc aufgerufen, soll ja ein neuer const char* zurück gegeben werden. malloc versucht sich ein neues Häppchen Speicher aus dem tcache zu holen (sind ja nur wenige bytes), aber stolpert dabei und printet einen Fehler + ruft wohl abort() aus stdlib.h auf.
-
Bin kein Programmierer, aber versucht doch einfach mal statt readlock zum Ursache finden die Channels zu duplizieren so dass jeder Thread seine eigene Kopie hat. Dann kann es ja keine doppelten Zugriffe mehr geben. Ist nur zum Fehler ausschließen, nicht für später.
Die Kanalliste "gehört" dem VDR, um die lesen zu dürfen muss man sich einen readlock holen, sonst ist Chaos vorprogrammiert.
Da sowohl die originale Variante, als auch die wirbel, als auch meine, das exakt gleich Verhalten zeigen, wird es am readlock selber wohl eher nicht liegen.
Man könnte testweise den VDR so modifizieren, dass nur ein Thread readlock auf die Channels gewährt bekommt, das müsste vergleichbar einfach machbar sein.
So würde dann nur noch einer der VNSI-Plugin-Threads gleichzeitig laufen.
Die Änderung würde einiges durcheinander werfen, was das gesamte Timing im VDR betrifft. Unter Umständen geht damit dann gar nichts mehr.
[...] (war doppelt)
writing 1 byte into .... und cc1plus : dest... is likely at address zero.
geben diese compiler Warnungen nicht Hinweise auf das Fehlverhalten?Leider nicht.
Das ist auch an einer völlig anderen Stelle, der Code wird hier gar nicht verwendet.
ich hab jetzt mal mit -O0 übersetzt, dann gibt es bei mir auch keine warnings ebenso keine mit
-Wno-stringop-overflow
. Es crasht dennoch:Dass es mit der Optimierung zu tun hat, habe ich schon öfters gelesen.
Vermutlich liegt es am am inlinen, das dürfte -O0 nicht machen.
Die Warnung ist aber generell wohl fehlerhaft, wenn man den GCC-Entwicklern trauen kann.
(Siehe Link in #69.) Die sind sich da auch einig, dass man die Warnung entfernen will.
Kann also gut sein, dass man das inzwischen gemacht hat und Warnung ab GCC Version 12.5 oder 12.6 verschwunden ist.
Generell ist dieses Konstrukt aber eher speziell (leerer Constructor dafür eine Initialisierungsliste), einen auffälligen Fehler sehe ich aber nicht.
Man könnte mal versuchen die Reihenfolge in der Initialisierungsliste zu tauschen, vielleicht bringt das was.
Patch im Anhang.(Entfernt, da es nicht funktioniert hat!)#1 0x00007f9fe0041176 __GI_raise (libc.so.6 + 0x41176)
#2 0x00007f9fe0028917 __GI_abort (libc.so.6 + 0x28917)
heißt aber AFAIK, dass das geplant aborted wurde, also dass ggf. das kaputt ist, was in den vasprintf gesteckt wurde.Prinzipiell richtig, nur kommen die Meldung vom *alloc() nicht vasprintf(), wie wirbel auch schon geschrieben hat.
Der Beitrag hat mich veranlasst mir endlich mal malloc() näher anzusehen, das hätte ich früher tun sollen...
Die Fehlermeldung malloc(): unaligned tcache chunk detected kommt von hier: malloc.c:3183
Wie man unschwer sieht, kommt die, wenn mit dem Zeiger auf den Speicherbereich etwas nicht stimmt.
Nach dem "Durchklicken" einiger der Links, sieht es mir so aus, als ob das beim anfordern eines neuen Speicherblocks passiert und die Safe-Linking Pointer Protection anschlägt.
100% sicher bin ich da zwar nicht, die Ursache müsste aber immer sein, dass jemand ausserhalb seines angeforderten Speichers etwas geschrieben hat.
Der Tcache enthält kürzlich freigegebene Blöcke und ist Thread bezogen, jeder hat seinen eigenen.
Ob man daraus aber schlussfolgern kann, dass der Übeltäter im gleichen Thread sitzt, bin ich nicht sicher.
Wenn man nach "tcache" sucht findet man, dass es auch Attacken darauf gibt.
Ich denke, unser Fehler besteht schon lange, fällt aber erst jetzt durch eine neu eingeführte Abwehrmaßnahme auf.
Primär verdächtig ist alles was mit dynamischer Speicherverwaltung (malloc() und Co.) zu tun hat.
Die Funktionen von cResponsePacket machen das dauernd.
Ich habe da mal etwas Puffer eingebaut und hole den Speicher in größeren Blöcken. Das löst das Problem zwar nicht, sollte die Wahrscheinlichkeit für einen Crash aber senken.
Wenn es mit den Patch besser wird, weiß man also wenigstens, wo man suchen muss.
-
Code
if( (80 + bufUsed + by) < bufSize) ^ ? -
Wenn ich irgendwie helfen kann, dann bitte melden. Ich sehe jetzt erstmal noch davon ab Patches ins Repo zu übernehmen. Ansonsten mache ich erstmal den stillen Zuschauer und verfolge die Gedankengänge derer, die sich mit C++ definitiv deutlich besser auskennen als ich.
-
Die Patche lassen sich anwenden, ich erhalte aber dann folgenden compile ERROR:
Beim responsepacket-Patch ist was durcheinander geraten, irgendwie hatte es die neueste Version nicht bis zum diff.txt geschafft.
Wenn man alles umbenennen muss, um es anzuhängen, muss das wohl mal passieren, sorry.
Ist jetzt oben aber korrigiert.
der VDR crasht jetzt sofort beim start:
Das ist eine Folge hier von:
Codevideoinput.c: In constructor ‘cDummyReceiver::cDummyReceiver()’: videoinput.c:439:21: warning: ‘cDummyReceiver::m_BeenDetached’ will be initialized after [-Wreorder] 439 | std::atomic<bool> m_BeenDetached; | ^~~~~~~~~~~~~~ videoinput.c:440:72: warning: base ‘cReceiver’ [-Wreorder] 440 | cDummyReceiver() : m_BeenDetached(false), cReceiver(NULL, MINPRIORITY) {} | ^ videoinput.c:440:3: warning: when initialized here [-Wreorder] 440 | cDummyReceiver() : m_BeenDetached(false), cReceiver(NULL, MINPRIORITY) {} | ^~~~~~~~~~~~~~Und das ist eine Folge des "videoinput-Patches", den also bitte nicht mehr anwenden!
Ich hatte schon vermutet, dass man die Reihenfolge nicht einfach ungestraft ändern kann, bekam aber keine Warnung.
Irgendwie war bei mir das "-Wall" Flag verschwunden und ich habe es nicht gemerkt. Jetzt habe ich die Warnung aber auch.
Der Rest sind die bekannten Kandidaten, der Patch aus #81 fehlt vermutlich.
Den nächsten Versuch bitte mit dem Patch aus #81 und dem (neuen!) responsepacked-realloc3 aus #115.
Der aus #81 behebt die Warnungen und der aus #115 ist zum testen.
Der Rest der Patches hat ja nichts gebracht, belastet IMHO also nur.
Kann also gut sein, dass man das inzwischen gemacht hat und Warnung ab GCC Version 12.5 oder 12.6 verschwunden ist.
Kann doch nicht sein, Version 12.4 ist noch nicht draußen. Fix ist aber dafür vorgesehen.
Bei Version 13.2 kann es aber sein, da kann es zeitlich hin kommen.
Leider konnte ich aber nichts genaueres finden. Die Suche nach der Bug-Nummer im GIT brachte auch nichts.
Jetzt mitmachen!
Sie haben noch kein Benutzerkonto auf unserer Seite? Registrieren Sie sich kostenlos und nehmen Sie an unserer Community teil!