sectionsd: "use of uninitialized value of size 8"

[seife]

Ich habe heute mal sectionsd mit valgrind "bearbeitet".
Dabei kam einiges in dieser Art zum Vorschein:

Code: Alles auswählen

==19941== Use of uninitialised value of size 8
==19941==    at 0x43BF81: SIsectionEIT::parseDescriptors(char const*, unsigned, SIevent&) (SIsections.cpp:291)
==19941==    by 0x43C0FF: SIsectionEIT::parse() (SIsections.cpp:321)
==19941==    by 0x432875: SIsectionEIT::SIsectionEIT(unsigned, char*) (SIsections.hpp:475)
==19941==    by 0x42067F: _ZL9eitThreadPv (sectionsd.cpp:7076)
==19941==    by 0x568106F: start_thread (in /lib64/libpthread-2.9.so)
==19941==    by 0x60E510C: clone (in /lib64/libc-2.9.so)

ich habe es auf das hier zurückgeführt, bin mir aber nicht sicher:

Code: Alles auswählen

diff --git a/tuxbox/neutrino/daemons/sectionsd/SIsections.cpp b/tuxbox/neutrino/daemons/sectionsd/SIsections.cpp
index 0bf720d..f6944c6 100644
--- a/tuxbox/neutrino/daemons/sectionsd/SIsections.cpp
+++ b/tuxbox/neutrino/daemons/sectionsd/SIsections.cpp
@@ -312,7 +312,7 @@ void SIsectionEIT::parse(void)
                e.original_network_id = original_network_id();
                e.transport_stream_id = transport_stream_id();
                descriptors_loop_length = (evt->descriptors_loop_length_hi << 8) | evt->descriptors_loop_length_lo;
-               parseDescriptors(((const char *)evt) + sizeof(struct eit_event), min((unsigned)(buffer + bufferLength - actPos), descriptors_loop_lengt
h), e);
+               parseDescriptors(((const char *)evt) + sizeof(struct eit_event), min((unsigned)(buffer + bufferLength - actPos - sizeof(struct eit_even
t)), descriptors_loop_length), e);
                evts.insert(e);
                actPos += sizeof(struct eit_event) + descriptors_loop_length;
        }

Ich bin mir aber nicht sicher, ob das so richtig ist.
Ich vermute, da wir ja parseDescriptors() den speicher ab der stelle (evt + sizeof eit_event) übergeben, müssen wir auch sizeof eit_event von der Länge abziehen.
Stimmt das?
Damit meckert valgrind zumindest nicht mehr rum...

[GetAway]

Tja, wenn Du das nicht weisst, wer soll es dann wissen.

Was würde denn passieren, wenn Du es kaputt gemacht hast?
Im Notfall würde ich es mal testen und beobachten.

[seife]

Bisher ist es auf jeden Fall kaputt. (Der sectionsd ist mir in kurzer Zeit um die Ohren geflogen, als ich ihn auf x86_64, mit MALLOC_CHECK_=2 probiert habe).

Der Speicher, auf den da zugegriffen wird, liegt nicht so weit "ausserhalb", dass es für einen segfault "reicht".
MALLOC_CHECK_ macht aber zusätzliche checks, und beendet sich falsch beendende Programme.
valgrind hat mir dann gesagt, dass der Puffer um 8 Bytes "überschritten" wurde. Das ist zufällig genau die Grösse von struct eit_event.

für mich sieht es so aus, als würden wir (pseudocode) das machen:

Code: Alles auswählen

// buf ist ein pointer auf einen Puffer mit Größe bufLen
    parseDescriptors(buf + 8, bufLen); // parsed den Puffer, dessen pointer übergeben wurde

Das würde natürlich, da wir "buf + 8" übergeben, dann im parseDescriptors übers Ziel hinausschiessen.
Dummerweise ist der echte Code nicht so übersichtlich wie mein Pseudocode.
Was passieren könnte, falls ich falsch liege? Irgendwelche Deskriptoren würden halt niemals eingelesen, weil sie nie gefunden würden. Ob man das merken würde, weiss ich nicht. Evtl. kommen sie ja später einfach wieder, an anderer Stelle im Puffer...

Ich habe ab und zu auf dem ORF2E "abgehacktes" EPG, das könnte theoretisch damit zu tun haben (nämlich damit, dass _jetzt_ Müll geparsed wird).

[Houdini]

@seife: ich gebe dir recht.

Das ist ja dasselbe:

Code: Alles auswählen

		parseDescriptors(((const char *)evt) + sizeof(struct eit_event), min((unsigned)(buffer + bufferLength - (((const char *)evt) + sizeof(struct eit_event))), descriptors_loop_length), e);

[GetAway]

Habs seit 2 Stunden am laufen, scheint noch zu funktionieren.

[seife]

Ich habe es etwas anders gelöst, und zwar so:

Code: Alles auswählen

@@ -263,6 +267,8 @@ void SIsectionEIT::parseShortEventDescriptor(const char *buf, SIevent &e, unsign

 void SIsectionEIT::parseDescriptors(const char *des, unsigned len, SIevent &e)
 {
+       des += sizeof(struct eit_event);
+       len -= sizeof(struct eit_event);
        struct descr_generic_header *desc;
        while(len>=sizeof(struct descr_generic_header)) {
                desc=(struct descr_generic_header *)des;
@@ -289,7 +295,7 @@ void SIsectionEIT::parseDescriptors(const char *des, unsigned len, SIevent &e)
 // Die infos aus dem Puffer holen
 void SIsectionEIT::parse(void)
 {
-       const char *actPos;
+       size_t actPos;
        struct eit_event *evt;
        unsigned short descriptors_loop_length;

@@ -298,23 +304,23 @@ void SIsectionEIT::parse(void)

        if (bufferLength < sizeof(SI_section_EIT_header) + sizeof(struct eit_event)) {
                delete [] buffer;
-               buffer=0;
+               buffer = NULL;
                bufferLength=0;
                return;
        }

-       actPos = &buffer[sizeof(SI_section_EIT_header)];
+       actPos = sizeof(SI_section_EIT_header);

-       while (actPos < &buffer[bufferLength - sizeof(struct eit_event)]) {
-               evt = (struct eit_event *) actPos;
+       while (actPos < bufferLength - sizeof(struct eit_event)) {
+               evt = (struct eit_event *)(buffer + actPos);
                SIevent e(evt);
                e.service_id = service_id();
                e.original_network_id = original_network_id();
                e.transport_stream_id = transport_stream_id();
-               descriptors_loop_length = (evt->descriptors_loop_length_hi << 8) | evt->descriptors_loop_length_lo;
-               parseDescriptors(((const char *)evt) + sizeof(struct eit_event), min((unsigned)(buffer + bufferLength - actPos), descriptors_loop_length), e);
+               descriptors_loop_length = sizeof(struct eit_event) + ((evt->descriptors_loop_length_hi << 8) | evt->descriptors_loop_length_lo);
+               parseDescriptors(buffer + actPos, min(bufferLength - actPos, descriptors_loop_length), e);
                evts.insert(e);
-               actPos += sizeof(struct eit_event) + descriptors_loop_length;
+               actPos += descriptors_loop_length;
        }

        parsed = 1;

Ich finde das besser lesbar, aber es gehört wohl noch ein Kommentar rein, warum der eit_event Header mit übergeben, aber dann gleich übersprungen wird.
Und dann muss man das in den anderen parseDescriptors()-Funktionen auch noch machen.
Wenn ich am Wochenende dazukomme, probiere ich es aus und checke es ein. Ganz so dringend ist es ja nicht, schliesslich hat es die letzten 6 Jahre auch ohne funktioniert ;-)

Ansonsten hat sich sectionsd im valgrind erstaunlich wacker geschlagen, ich habe keine offensichtlichen leaks gefunden, die Mär vom sectionsd der leaked wie sau kann ich also nach objektiver Untersuchung so nicht bestätigen.

...allerdings war mein testcase nicht ganz praxisgerecht, da es keinen Client (neutrino) gab, der auf den sectionsd zugegriffen hätte.

[Striper]

Ansonsten hat sich sectionsd im valgrind erstaunlich wacker geschlagen, ich habe keine offensichtlichen leaks gefunden, die Mär vom sectionsd der leaked wie sau kann ich also nach objektiver Untersuchung so nicht bestätigen.

Naja die leaks hast du ja alle schon gestopft mittlerweile. Da kann man leicht mal von ner Mär reden. ;) Das einzige was imo noch Lecks hat ist "MP1" und der Audioplayer.