[vz-dev] vzlogger segfault (was: ] IR-KOPF und Y-Port Node)

[Thorben Thuermer]

(mit CC an steffen (vzlogger) und juri (libsml)...)

On Sun, 14 Oct 2012 13:36:25 +0200
Jürgen Herrmann <juergen.herrmann at plugid.de> wrote:
> Kann es, das eine Variable nicht beim Start initaliert wird...
ganz so einfach ist das leider nicht, aber... (s.u.)

> auch schreibt es keine Daten in die Datenbank .....
das ist ein anderes problem und liegt eher an einem fehler in deiner
konfiguration...

nochmal ganz deutlich:
das gesamte verhalten:
* keine daten werden geloggt
* der absturz unten
deutet daruf hin, das die daten die von deinem zaehler kommen
(fuer den sml-parser) voelliger muell sind,
deswegen koennen keine zu loggenden werte ermittelt werden,
und irgendwann kommt dann eine sequenz, die der parser
nicht verarbeiten kann und es fuehrt (aufgrund von bugs!) zum crash.
(es ist die frage, wieviel zeit man da reinstecken will,
 ungueltige daten "korrekt" zu verarbeiten, aber auf jeden
 fall sollte man unkontrollierte abstuerze vermeiden.)

> hier mal eine Terinal ausgabe

der backtrace ist SUPER, wir kommen dem problem naeher:

> gdb --args vzlogger -f -v20 --config /etc/vzlogger.conf
> (gdb) bt full
> #0 0xb7e5a367 in memset () from /lib/i686/cmov/libc.so.6
> No symbol table info available.
> #1 0x08051237 in sml_buffer_init (length=4294967280) at src/sml_shared.c:118
> buf = 0x8066af0

sml_buffer_init alloziert einen pufferm mit der uebergebenen groesse.
es wird eine VIEL zu grosse groesse uebergeben (mehr unten),
4294967280 = 4 gigabyte,
malloc schlaegt fehl und liefert 0,
aber der rueckgabewert von malloc wird nicht geprueft,
sondern es wird versucht ueber den nullpointer zu schreiben
=> segfault.

ein erster fix an der stelle:
sml_buffer *sml_buffer_init(size_t length) {
        sml_buffer *buf = (sml_buffer *) malloc(sizeof(sml_buffer));
+       if (!buf) return null;

> #2 0x080506cb in sml_file_parse (buffer=0xb791a1b8 "", 
> buffer_len=4294967280)
> at src/sml_file.c:35

diese funktion ist recht unschuldig, sollte dann aber auch die
rueckgabe von sml_buffer_init pruefen und bei fehlern abbrechen.

sml_file *sml_file_parse(unsigned char *buffer, size_t buffer_len) {
        sml_file *file = (sml_file*) malloc(sizeof(sml_file));
        memset(file, 0, sizeof(sml_file));

        sml_buffer *buf = sml_buffer_init(buffer_len);
+       if (!buf){ free file; return null;}

> #3 0x0804fd26 in meter_read_sml (meter=0x8066ca0, rds=0x8067510, n=32)
> at protocols/sml.c:163
> buffer = '\000' <repeats 7660 times>, 
> "h\272\336\267\020ii\rܿ\221\267f\274\376\267\217\210߷Y\231\004\b\000\000\000\000ە\004\b\030\365\222\267\002\000\221\267\340\030\377\267ە\004\bh\272\371\267\364\357\377\267\230\200\336\267\a\000\000\000\\\300\221\267\001\303\376\267", 
[...]
> bytes = 0
> m = <value optimized out>
> file = 0x0

und hier ist wohl der uebeltaeter:
159         /* wait until a we receive a new datagram from the meter
160         bytes = sml_transport_read(handle->fd, buffer,
SML_BUFFER_LEN);
161
162         /* parse SML file & stripping escape sequences */
163         file = sml_file_parse(buffer + 8, bytes - 16);

es wird als puffergroesse bytes - 16 an sml_file_parse uebergeben,
aber bytes ist wohl kleiner als 16, damit gibt es einen unterlauf/
wraparound bei der subtraktion, und wir bekommen die viel zu
grosse puffergroesse!

sinnvoll waehre da ein:
160         bytes = sml_transport_read(handle->fd, buffer,
SML_BUFFER_LEN);
+           if (bytes <= 16) {
+                // warnung ausgeben und abbrechen
und:
163         file = sml_file_parse(buffer + 8, bytes - 16);
+           if (!file) { // ditto

kenne mich fuer die details leider nicht gut genug im code aus,
aber der STEFFEN nimmt sich dessen sicher gerne an!

und vielleicht sollte auch in sml_transport_read() noch eine
pruefung gemacht werden, ob die gelesenen daten lang genug fuer eine
gueltige sml-nachricht sind?

- T.