unerklärlicher SIGSEGV

René H. schrieb:
> sprintf(dateStr, "%02d.%02d.%04d", my_tm->tm_mday,
> my_tm->tm_mon+1, my_tm->tm_year+1900);
>          sprintf(timeStr, "%02d:%02d:%02d", my_tm->tm_hour,
> my_tm->tm_min, my_tm->tm_sec);

Wenn du da schon rumprinten musst, nimm wenigstens snprintf oder sowas.
Wenn da Schrott in der struct steht, schreibst das so locker über die 
bounds. Sträflicher code.

Ansonsten empfehle ich valgrind oder einen sanitizer.

Ja, wo ist denn das return o;

Außerdem: assert(my_tm != nullptr);

Wilhelm M. schrieb:
> Ja, wo ist denn das return o;
>
> Außerdem: assert(my_tm != nullptr);

Aaaargh!!! Thx.

Grüsse,
René

Heiko L. schrieb:
> René H. schrieb:
> sprintf(dateStr, "%02d.%02d.%04d", my_tm->tm_mday, my_tm->tm_mon+1,
> my_tm->tm_year+1900);
>          sprintf(timeStr, "%02d:%02d:%02d", my_tm->tm_hour,
> my_tm->tm_min, my_tm->tm_sec);
>
> Wenn du da schon rumprinten musst, nimm wenigstens snprintf oder sowas.
> Wenn da Schrott in der struct steht, schreibst das so locker über die
> bounds. Sträflicher code.
>
> Ansonsten empfehle ich valgrind oder einen sanitizer.

Egal welcher Schrott da steht (was man bei gmtime() getrost ignorieren 
kann, sind die Bounds terminiert). Nicht das Problem.

René H. schrieb:
> Egal welcher Schrott da steht (was man bei gmtime() getrost ignorieren
> kann, sind die Bounds terminiert). Nicht das Problem.

Ja, 32 bit? -2147483648 = 11 Zeichen * 3 + 3 = 36.
Mit sprintf sagt man halt: "Schreib hin wo du willst."
Da kann man sicher auf den Exploit warten, der über einen Hack der 
Date-API root-Zugriff erzeugt.

Heiko, wenn Du es schaffst von aussen das Firmennetzwerk zu hacken und 
den Exploit zu setzen, bist du ein gemachter Mann für den Rest Deines 
Lebens.

Gelingt es aber nicht (was sehr wahrscheinlich ist), landest Du ob dem 
ersten Versuch im Knast. ;-)

So, have fun.

René H. schrieb:
> Heiko, wenn Du es schaffst von aussen das Firmennetzwerk zu hacken
> und
> den Exploit zu setzen, bist du ein gemachter Mann für den Rest Deines
> Lebens.
>
> Gelingt es aber nicht (was sehr wahrscheinlichist), landest Du ob dem
> ersten Versuch im Knast. ;-)
>
> So, have fun.

Ich meine nur. Solche ungeprüften writes sind DER Einstiegspunkt 
schlechthin. Schon als ich damals C gelernt habe, stand das in jedem 
Tutorial. "Nie, nie, niemals, nie einfach sprintf."

René H. schrieb:
> Wilhelm M. schrieb:
>> Ja, wo ist denn das return o;
>>
>> Außerdem: assert(my_tm != nullptr);
>
> Aaaargh!!! Thx.
>
> Grüsse,
> René

Wohl mal wieder keine Warnungen eingeschaltet?

Heiko L. schrieb:
> René H. schrieb:
>> Heiko, wenn Du es schaffst von aussen das Firmennetzwerk zu hacken
>> und
>> den Exploit zu setzen, bist du ein gemachter Mann für den Rest Deines
>> Lebens.
>>
>> Gelingt es aber nicht (was sehr wahrscheinlichist), landest Du ob dem
>> ersten Versuch im Knast. ;-)
>>
>> So, have fun.
>
> Ich meine nur. Solche ungeprüften writes sind DER Einstiegspunkt
> schlechthin. Schon als ich damals C gelernt habe, stand das in jedem
> Tutorial. "Nie, nie, niemals, nie einfach sprintf."

Ja, wenn sie unformatiert sind, in dem Fall sind sie formatiert.

Grüsse,
René

René H. schrieb:
> Ja, wenn sie unformatiert sind, in dem Fall sind sie formatiert.

Du gibst aber nur die Minimalfeldgröße an mit Füllzeichen. Es kann auch 
größer werden. Da gibt es den Modulo-trick.

> Ja, wenn sie unformatiert sind, in dem Fall sind sie formatiert.
>
> Grüsse,
> René

Also wenn du dir sicher sein kannst, dass der Code nie auf 32bit läuft, 
dann kann man doch auch sicher sein, dass ein %s-Eingabe-String nie 
länger als X Zeichen ist, oder nicht?

Nichts für ungut.

Heiko L. schrieb:
>> Ja, wenn sie unformatiert sind, in dem Fall sind sie formatiert.
>>
>> Grüsse,
>> René
>
> Also wenn du dir sicher sein kannst, dass der Code nie auf 32bit läuft,
> dann kann man doch auch sicher sein, dass ein %s-Eingabe-String nie
> länger als X Zeichen ist, oder nicht?
>
> Nichts für ungut.

Wo siehst Du %s? Der Code läuft auf einem 64Bit / 48 CPU System.

Wilhelm M. schrieb:
> René H. schrieb:
>> Ja, wenn sie unformatiert sind, in dem Fall sind sie formatiert.
>
> Du gibst aber nur die Minimalfeldgröße an mit Füllzeichen. Es kann auch
> größer werden. Da gibt es den Modulo-trick.

Nope, kann es nicht. Sonst zeig mir wie .....

René H. schrieb:
> Heiko L. schrieb:
>>> Ja, wenn sie unformatiert sind, in dem Fall sind sie formatiert.
>>>
>>> Grüsse,
>>> René
>>
>> Also wenn du dir sicher sein kannst, dass der Code nie auf 32bit läuft,
>> dann kann man doch auch sicher sein, dass ein %s-Eingabe-String nie
>> länger als X Zeichen ist, oder nicht?
>>
>> Nichts für ungut.
>
> Wo siehst Du %s? Der Code läuft auf einem 64Bit / 48 CPU System.

Nein, das war eine Analogie: Ein sprintf(stack, "%s", something) ist 
offensichtlicher irgendwie gefährlich. Bei %Xd hat man immerhin noch 
eine gewisse Größenvorstellung. Doch wie Wilhelm schon sagt

1

(number)  Minimum number of characters to be printed. ...

"Es können durchaus mehr werden."
Da ist man entweder so hart, zu sagen "Auf den Architekturen, wo das 
laufen wird ist der Zahlentyp maximal so und so lang" (was dann 
irgendwann kaputt geht, wenn der Code in weniger heimischer Umgebung 
herumstreunt) oder man sichert das halt von vornherein ab.

Im vorliegenden Fall: Warum nicht direkt mit dem stream arbeiten?

René H. schrieb:
> Nope, kann es nicht. Sonst zeig mir wie .....

Sicher:

1

printf("%02d\n", 12345678);

Wilhelm M. schrieb:
> René H. schrieb:
> Nope, kann es nicht. Sonst zeig mir wie .....
>
> Sicher:printf("%02d\n", 12345678);

Yep, sicher!

Getestet.

Grüsse,
René

Ich denke aber dein Hinweis wird es gewesen sein, leider kann ich erst 
ab morgen Abend testen da kein Input übers WE.

Heiko L. schrieb:
> René H. schrieb:
>> Heiko L. schrieb:
>>>> Ja, wenn sie unformatiert sind, in dem Fall sind sie formatiert.
>>>>
>>>> Grüsse,
>>>> René
>>>
>>> Also wenn du dir sicher sein kannst, dass der Code nie auf 32bit läuft,
>>> dann kann man doch auch sicher sein, dass ein %s-Eingabe-String nie
>>> länger als X Zeichen ist, oder nicht?
>>>
>>> Nichts für ungut.
>>
>> Wo siehst Du %s? Der Code läuft auf einem 64Bit / 48 CPU System.
>
> Nein, das war eine Analogie: Ein sprintf(stack, "%s", something) ist
> offensichtlicher irgendwie gefährlich. Bei %Xd hat man immerhin noch
> eine gewisse Größenvorstellung. Doch wie Wilhelm schon sagt
>

1

> (number)  Minimum number of characters to be printed. ...

2

>

> "Es können durchaus mehr werden."
> Da ist man entweder so hart, zu sagen "Auf den Architekturen, wo das
> laufen wird ist der Zahlentyp maximal so und so lang" (was dann
> irgendwann kaputt geht, wenn der Code in weniger heimischer Umgebung
> herumstreunt) oder man sichert das halt von vornherein ab.
>
> Im vorliegenden Fall: Warum nicht direkt mit dem stream arbeiten?

Ich bau es nach sstream um, war bloss zu faul. Danke für die Hinweise!

Grüsse,
René

Wilhelm M. schrieb:
> René H. schrieb:
>> Nope, kann es nicht. Sonst zeig mir wie .....
>
> Sicher:
> printf("%02d\n", 12345678);

Mist!!! Ihr habt Recht :-(

1

int main(int argc, char** argv)

2

{

3

   char str[32];

4

5

   sprintf(str, "%02d", 123456);

6

7

   std::cout << str << std::endl;

8

9

   return(0);

10

}

1

xxx@yyyyyy:~/tmp> ./overwrite 

2

123456

Grüsse,
René

René H. schrieb:
> sprintf(str, "%02d", 123456);

Da gibt es den Modulo-Trick:

1

sprintf(str, "%02d", 123456 % 100);

Wilhelm M. schrieb:
> René H. schrieb:
>> sprintf(str, "%02d", 123456);
>
> Da gibt es den Modulo-Trick:
> sprintf(str, "%02d", 123456 % 100);

Das ist gut, ich werde es aber trotzdem auf sstream umbauen....

Aber (so hoffe ich), dass das fehlende Return zum SIGSEGV geführt hat 
(ich kann es mir nicht vorstellen, möglich ist es dennoch). Ich sehe 
sonst keine Möglichkeit für Speicherüberschreibungen (muss dem sprintf, 
wie ich ja jetzt weiss).


Grüsse,
René

René H. schrieb:
> Aber (so hoffe ich), dass das fehlende Return zum SIGSEGV geführt hat
> (ich kann es mir nicht vorstellen, möglich ist es dennoch).

auf jeden Fall

René H. schrieb:
> Aber (so hoffe ich), dass das fehlende Return zum SIGSEGV geführt hat
> (ich kann es mir nicht vorstellen, möglich ist es dennoch).

Also ich kann bestätigen, dass fehlende returns mitunter SEGVs erzeugen:
Das ist mir nämlich auch schon passiert. (Dabei hatte ich aber sogar 
noch warnings ignoriert shame.)
Wird mal Zeit, -Werror zu benutzen.

Wilhelm M. schrieb:
> René H. schrieb:
>> Wilhelm M. schrieb:
>>> Ja, wo ist denn das return o;
>>>
>>> Außerdem: assert(my_tm != nullptr);
>>
>> Aaaargh!!! Thx.
>>
>> Grüsse,
>> René
>
> Wohl mal wieder keine Warnungen eingeschaltet?

Nope! Die ärgern doch bloss :-)

foobar schrieb:
> Es wurde schon genannt, dass gmtime ein NULL zurückliefern kann.  Ein
> weiteres Problem angesichts des "ThreadControl" ist, dass gmtime nicht
> Thread-safe ist - dafür gibt es gmtime_r.

Hmm..., das ist im Main Thread und sollte somit kein Problem darstellen.
Aber das assert hab ich eingebaut.

René H. schrieb:
> foobar schrieb:
>> Es wurde schon genannt, dass gmtime ein NULL zurückliefern kann.  Ein
>> weiteres Problem angesichts des "ThreadControl" ist, dass gmtime nicht
>> Thread-safe ist - dafür gibt es gmtime_r.
>
> Hmm..., das ist im Main Thread und sollte somit kein Problem darstellen.
> Aber das assert hab ich eingebaut.

Das ist immer ein Problem (sorry, hatte den Parameter gar nicht gesehen 
und daher nicht auf MT geschlossen).

Weißt Du überhaupt, was die anderen Threads machen?

Diese alten, zustandsbehafteten Funktionen sollte man überhaupt nicht 
mehr benutzen. Die kommen aus einer Zeit, als Posix/*nix nur 
heavyweight-processes kannte.

da gibt's doch nix rumzudeuteln.

Wenn man << überlädt und keinen vernünftigen Returnwert zurückgibt, muß 
die (meist folgende) Verkettung:

1

std::cout << obj << irgendwas;

praktisch zwangsläufig auf die Nase fallen. Die ist auf einen validen 
Returnwert angewiesen.

Oje schrieb:
> Aha. Und das tut sie dann nur im Debugmodus zwangslaeufig nicht?

ein nicht vorhandener, aber notwendiger Returnwert ist UB.

UB kann alles bedeuten, auch ein (zufällig) nicht ganz falscher Wert im 
(zufällig) richtigen Register, insofern ist es absolut müssig, darüber 
zu spekulieren.

Brauchen wir nicht. Steht gaaaanz oben.

René H. schrieb:
> foobar schrieb:
>> Es wurde schon genannt, dass gmtime ein NULL zurückliefern kann.  Ein
>> weiteres Problem angesichts des "ThreadControl" ist, dass gmtime nicht
>> Thread-safe ist - dafür gibt es gmtime_r.
>
> Hmm..., das ist im Main Thread und sollte somit kein Problem darstellen.

In welchem Thread das läuft, ist egal. Sobald irgendein anderer Thread 
zur gleichen Zeit gmtime() aufruft, hast du ein Problem. Wenn du 
garantieren kannst, dass das niemals nie nicht ein anderer Thread tun 
wird, dann ist alles ok.

Rolf M. schrieb:
> In welchem Thread das läuft, ist egal. Sobald irgendein anderer Thread
> zur gleichen Zeit gmtime() aufruft, hast du ein Problem. Wenn du
> garantieren kannst, dass das niemals nie nicht ein anderer Thread tun
> wird, dann ist alles ok.

Kann ich garantieren. Es werden 120 Thread gestartet und die berechnen 
die Latency. Bei SIGTERM werden alle gestoppt und ein CSV geschrieben.

ABER: da fehlten einige return's. Ich habe alles korrigiert und werde 
heute Abend mal testen.

Grüsse,
René

PS: Fcuk! Ich rufe localtime in den Threads auf. Das ist das Problem. 
:-(

René H. schrieb:
> ABER: da fehlten einige return's. Ich habe alles korrigiert und werde
> heute Abend mal testen.

Wie oben schon gesagt: das ist der Grund.


>
> PS: Fcuk! Ich rufe localtime in den Threads auf. Das ist das Problem.
> :-(

Das ist zwar blöd, weil Du ggf. Dir Deinen Puffer zerstörst, aber es ist 
nicht das Problem des SEGV. Versuch macht kluch.

Und schalt die Warnungen ein, und bau Zusicherungen ein oder 
Fehlerbehandlung. So ist das jedenfalls nix.

Wilhelm M. schrieb:
> Das ist zwar blöd, weil Du ggf. Dir Deinen Puffer zerstörst, aber es ist
> nicht das Problem des SEGV.

Hinge das nicht von der Implementierung ab?

Heiko L. schrieb:
> Hinge das nicht von der Implementierung ab?

Genau, deswegen steht da ggf.

Wilhelm M. schrieb:
> Heiko L. schrieb:
>> Hinge das nicht von der Implementierung ab?
>
> Genau, deswegen steht da ggf.

Naja, der alte Buffer könnte ja auch free'd werden (je, nachdem in 
welcher Referenz man nachschlägt).

Wilhelm M. schrieb:
> Das ist zwar blöd, weil Du ggf. Dir Deinen Puffer zerstörst, aber es ist
> nicht das Problem des SEGV.

Das kann ich ausschliessen und habe ich bedacht. Ich habe alles 
hinzugefügt inkl. Fehlerhandling. Das brachte allerdings nichts :-(

strace sagt immer noch SIGSEV, es muss in den Threads liegen. Den Code 
kann/darf ich leider nicht zeigen.

Grüsse,
René

René H. schrieb:
> strace sagt immer noch SIGSEV, es muss in den Threads liegen. Den Code
> kann/darf ich leider nicht zeigen.

Alles schon gemacht?

- warnungen an
- returns eingebaut
- *_r Funktionen
- Zusicherungen

- Datenstrukturen synchronisiert

René H. schrieb:
> strace sagt immer noch SIGSEV, es muss in den Threads liegen. Den Code
> kann/darf ich leider nicht zeigen.

Hast du die Info, wo genau? Ansonsten versuche mal valgrind oder einen 
Sanitizer.

Wilhelm M. schrieb:
> René H. schrieb:
>> strace sagt immer noch SIGSEV, es muss in den Threads liegen. Den Code
>> kann/darf ich leider nicht zeigen.
>
> Alles schon gemacht?
>
> - warnungen an
> - returns eingebaut
> - *_r Funktionen
> - Zusicherungen
>
> - Datenstrukturen synchronisiert

Warnungen an sind Böse, ich habe es mal mit -Werror kompiliert. Das sind 
mehrere 100'000 Zeilen Code, damit bin ich erst mal beschäftigt.

Return's sind drin. _r Funktionen auch.

Was heisst Zusicherung? Datenstrukturen sind synchronisiert.

Grüsse,
René

PS: der code ist über 15 Jahre alt :-(

René H. schrieb:
> Was heisst Zusicherung?

Echt jetzt?

Assertionen.

In Deinem o.g. Code verwendest Du einen Datentyp (Zeigertyp), wobei Du 
im folgenden Code davon ausgehst, dass ein bestimmter Wert (nullptr), 
der ja zum Wertebereich gehört und von den Funktionen geliefert werden 
kann, nicht vorkommt. So etwas darf man nicht machen. Um dies (Wert != 
nullptr) für den folgenden Code zuzusichern, verwendet man Assertionen. 
Im Falle des Falles hast Du an genau der Zeile dann eben einen 
Zusicherungsfehler (Exception in C++).

Die Assertionen sind Macros und nicht drin, wenn Du mit -DNDEBUG 
compilierst.

René H. schrieb:
> Datenstrukturen sind synchronisiert.

Wie?

Wilhelm M. schrieb:
> Echt jetzt?
>
> Assertionen.

Dann sag doch gleich Assertions :-). Damit kann ich etwas machen, mit 
dem Deutschsprachigen Begriff bin ich nicht vertraut.

Yep, sind drin.

Wilhelm M. schrieb:
> Wie?

Jeder Thread hat seine eigene Datenstruktur, nach beenden der Threads 
werden die eingesammelt. Da gibt es nichts zu synchronisieren. Ab 18:00 
Uhr kann ich testen.

René H. schrieb:
> _r Funktionen auch.

Beweise es!

René H. schrieb:
> Datenstrukturen sind synchronisiert.

René H. schrieb:
> Da gibt es nichts zu synchronisieren.

Na was denn jetzt.

Wilhelm M. schrieb:
> René H. schrieb:
>> _r Funktionen auch.
>
> Beweise es!
>
> René H. schrieb:
>> Datenstrukturen sind synchronisiert.
>
> René H. schrieb:
>> Da gibt es nichts zu synchronisieren.
>
> Na was denn jetzt.

Mann, Wilhelm, ich darf den Teil vom Code hier nicht zeigen (ich würde 
es liebend gern).

Es gibt nichts zu synchronisieren. Deshalb dass getLatencyMap. Die 
Threads sammeln und nach Beendigung der Threads sammelt der Main Thread 
alle maps ein und gibt sie als CSV aus. Jeder Thread ist für sich 
autonom. Keine Mutex nichts.

René H. schrieb:
> Mann, Wilhelm, ich darf den Teil vom Code hier nicht zeigen (ich würde
> es liebend gern).

Den will ich gar nicht sehen.

Nur sollst Du zeigen, dass eben die zustandsbehaftenen Versionen der *_r 
Funktionen nicht(!) vorkommen,

Wilhelm M. schrieb:
> Beweise es!

1

/* operator<<:

2

 * ------------------------------------------------------------------------- */

3

std::ostream& operator<< (std::ostream& o, const ThreadControl& c)

4

{

5

   latency_map::const_iterator it;

6

   readerMap_t::const_iterator itm;

7

8

9

   for (itm = c.reader_map_.begin(); itm != c.reader_map_.end(); ++itm) {

10

      QueueReader*   qr = itm->second.q_reader;

11

      assert (qr != nullptr);

12

13

      latency_map*   map = qr->getLatencyMap();

14

      assert (map != nullptr);

15

16

      for (it = map->begin(); it != map->end(); ++it) {

17

         uint32_t ts = it->first;

18

         const time_t  tv = (time_t)ts;

19

         struct tm * my_tm = gmtime(&tv);

20

         char     timeStr[32];

21

         char     dateStr[32];

22

23

         assert (my_tm != nullptr);

24

         sprintf(dateStr, "%02d.%02d.%04d", my_tm->tm_mday, my_tm->tm_mon+1, my_tm->tm_year+1900);

25

         sprintf(timeStr, "%02d:%02d:%02d", my_tm->tm_hour, my_tm->tm_min, my_tm->tm_sec);

26

27

         o << (uint16_t) itm->first << ";";

28

         o << dateStr << " " << timeStr << ";";

29

30

         o << it->second << std::endl;

31

      }

32

   }

33

34

   return(o);

35

}

Wilhelm M. schrieb:
> Nur sollst Du zeigen, dass eben die zustandsbehaftenen Versionen der *_r
> Funktionen nicht(!) vorkommen,

Im gesamten Code, also die 100.000 Zeilen

Da sehe ich immer noch gmtime()

So, Korrektur:

1

/* operator<<:

2

 * ------------------------------------------------------------------------- */

3

std::ostream& operator<< (std::ostream& o, const ThreadControl& c)

4

{

5

   latency_map::const_iterator it;

6

   readerMap_t::const_iterator itm;

7

8

9

   for (itm = c.reader_map_.begin(); itm != c.reader_map_.end(); ++itm) {

10

      QueueReader*   qr = itm->second.q_reader;

11

      assert (qr != nullptr);

12

13

      latency_map*   map = qr->getLatencyMap();

14

      assert (map != nullptr);

15

16

      for (it = map->begin(); it != map->end(); ++it) {

17

         uint32_t ts = it->first;

18

         const time_t  tv = (time_t)ts;

19

         struct tm * my_tm = gmtime_r(&tv, my_tm);

20

         char     timeStr[32];

21

         char     dateStr[32];

22

23

         assert (my_tm != nullptr);

24

         sprintf(dateStr, "%02d.%02d.%04d", my_tm->tm_mday, my_tm->tm_mon+1, my_tm->tm_year+1900);

25

         sprintf(timeStr, "%02d:%02d:%02d", my_tm->tm_hour, my_tm->tm_min, my_tm->tm_sec);

26

27

         o << (uint16_t) itm->first << ";";

28

         o << dateStr << " " << timeStr << ";";

29

30

         o << it->second << std::endl;

31

      }

32

   }

33

34

   return(o);

35

}

Wilhelm M. schrieb:
> also die

Mein Code sind lediglich ein paar hundert Zeilen und unabhängig davon. 
Aber es ist nun mal Bestandteil vom "grossen" Projekt.

Oh ha,

liest Du eigentlich Doku?

Jetzt hast Du sicher einen SEGV.

Du hast das Prinzip einer thread-lokalen Datenstruktur gar nicht 
verstanden. Ich befürchte schlimmes ...

Wilhelm M. schrieb:
> Oh ha,
>
> liest Du eigentlich Doku?
>
> Jetzt hast Du sicher einen SEGV.
>
> Du hast das Prinzip einer thread-lokalen Datenstruktur gar nicht
> verstanden. Ich befürchte schlimmes ...

Klär mich auf, bitte!

Wilhelm M. schrieb:
> Jetzt hast Du sicher einen SEGV

... und das mit dem -Werror scheint auch eher unwahrscheinlich

Ich kann beim besten Willen nicht sehen wo da ein SEGV sein soll.

René H. schrieb:
> Klär mich auf, bitte!

1

struct tm my_tm;

2

auto result = gmtime_r(&tv, &my_tm);

René H. schrieb:
> Ich kann beim besten Willen nicht sehen wo da ein SEGV sein soll.

uninitialisierter Zeiger my_tm;

Markus F. schrieb:
> ... und das mit dem -Werror scheint auch eher unwahrscheinlich

Wo ist der Zusammenhang zu SEGV jetzt hier?

Wilhelm M. schrieb:
> Du hast das Prinzip einer thread-lokalen Datenstruktur gar nicht
> verstanden. Ich befürchte schlimmes ...

Eigentlich schon... wo liegt mein Fehler?

René H. schrieb:
> Eigentlich schon... wo liegt mein Fehler?

s.a. mein Post gerade eben

my_tm wird direkt zugewiesen, da sehe ich nicht was nicht initialisiert 
sein soll.
auto kann ich nicht verwenden, wir haben noch c++98. Werfen also mit 
Steinen ;-)

René H. schrieb:
> my_tm wird direkt zugewiesen, da sehe ich nicht was nicht initialisiert
> sein soll.

Ja, aber wo zeigt es denn hin, beim Aufruf von gmtime()???
Eher ins Nirvana!

so ist dann besser :-)

1

/* operator<<:

2

 * ------------------------------------------------------------------------- */

3

std::ostream& operator<< (std::ostream& o, const ThreadControl& c)

4

{

5

   latency_map::const_iterator it;

6

   readerMap_t::const_iterator itm;

7

8

9

   for (itm = c.reader_map_.begin(); itm != c.reader_map_.end(); ++itm) {

10

      QueueReader*   qr = itm->second.q_reader;

11

      assert (qr != nullptr);

12

13

      latency_map*   map = qr->getLatencyMap();

14

      assert (map != nullptr);

15

16

      for (it = map->begin(); it != map->end(); ++it) {

17

         uint32_t ts = it->first;

18

         const time_t  tv = (time_t)ts;

19

         struct tm  my_tm;

20

         gmtime_r(&tv, &my_tm);

21

         char     timeStr[32];

22

         char     dateStr[32];

23

24

         assert (my_tm != nullptr);

25

         sprintf(dateStr, "%02d.%02d.%04d", my_tm.tm_mday, my_tm.tm_mon+1, my_tm.tm_year+1900);

26

         sprintf(timeStr, "%02d:%02d:%02d", my_tm.tm_hour, my_tm.tm_min, my_tm.tm_sec);

27

28

         o << (uint16_t) itm->first << ";";

29

         o << dateStr << " " << timeStr << ";";

30

31

         o << it->second << std::endl;

32

      }

33

   }

34

35

   return(o);

36

}

Also, jetzt ist die Methode definitiv Militärtauglich :-)

Nein!
Rückgabewert von gmtime_r() checken.

René H. schrieb:
> Also, jetzt ist die Methode definitiv Militärtauglich :-)

Nein, vorher war sie einfach nur falsch, jetzt ist sie (fast, s. Post 
hierüber) richtig.

1

/* operator<<:

2

 * ------------------------------------------------------------------------- */

3

std::ostream& operator<< (std::ostream& o, const ThreadControl& c)

4

{

5

   latency_map::const_iterator it;

6

   readerMap_t::const_iterator itm;

7

8

9

   for (itm = c.reader_map_.begin(); itm != c.reader_map_.end(); ++itm) {

10

      QueueReader*   qr = itm->second.q_reader;

11

      assert (qr != nullptr);

12

13

      latency_map*   map = qr->getLatencyMap();

14

      assert (map != nullptr);

15

16

      for (it = map->begin(); it != map->end(); ++it) {

17

         uint32_t ts = it->first;

18

         const time_t  tv = (time_t)ts;

19

         struct tm  my_tm, *result;

20

         result = gmtime_r(&tv, &my_tm);

21

         char     timeStr[32];

22

         char     dateStr[32];

23

24

         assert (result != nullptr);

25

         sprintf(dateStr, "%02d.%02d.%04d", my_tm.tm_mday, my_tm.tm_mon+1, my_tm.tm_year+1900);

26

         sprintf(timeStr, "%02d:%02d:%02d", my_tm.tm_hour, my_tm.tm_min, my_tm.tm_sec);

27

28

         o << (uint16_t) itm->first << ";";

29

         o << dateStr << " " << timeStr << ";";

30

31

         o << it->second << std::endl;

32

      }

33

   }

34

35

   return(o);

36

}

Militärtauglich?

Warum sind qr, ts, result und map non-const?

Weil ich per se faul bin?

René H. schrieb:
> Weil ich per se faul bin?

Dann ist es auch nicht militärtauglich ...

BTW: zeige, dass der restliche Code (100.00o Zeilen) , der nicht von Dir 
ist, auch keine zustandsbehaftenen Funktionen in den Thread verwendet.

Markus F. schrieb:
> Wilhelm M. schrieb:
>> Jetzt hast Du sicher einen SEGV
>
> ... und das mit dem -Werror scheint auch eher unwahrscheinlich

Wilhelm M. schrieb:
> Markus F. schrieb:
>> ... und das mit dem -Werror scheint auch eher unwahrscheinlich
>
> Wo ist der Zusammenhang zu SEGV jetzt hier?

Wenn tatsächlich mit -Werror compiliert würde, gäb's keinen, weil kein 
Binary.

René H. schrieb:
> struct tm * my_tm = gmtime_r(&tv, my_tm);

wo soll gmtime_r() deiner Ansicht nach sein Ergebnis hinschreiben?

Markus F. schrieb:
> wo soll gmtime_r() deiner Ansicht nach sein Ergebnis hinschreiben?

Hatten wir schon geklärt ;-)

Markus F. schrieb:
> Wenn tatsächlich mit -Werror compiliert würde, gäb's keinen, weil kein
> Binary.

Ja, Code der nicht compiliert, ist guter Code.

Mein Compiler ist const feindlich ;-)

1

/home/xxx/Proj/yyyy/tools/latency_measure/thread_control.cpp:82:44: error: passing \u2018const QueueReader\u2019 as \u2018this\u2019 argument of \u2018latency_map* QueueReader::getLatencyMap()\u2019 discards qualifiers [-fpermissive]

2

       latency_map* map = qr->getLatencyMap();

so ....

1

/* operator<<:

2

 * ------------------------------------------------------------------------- */

3

std::ostream& operator<< (std::ostream& o, const ThreadControl& c)

4

{

5

   latency_map::const_iterator it;

6

   readerMap_t::const_iterator itm;

7

8

9

   for (itm = c.reader_map_.begin(); itm != c.reader_map_.end(); ++itm) {

10

      QueueReader* const qr = itm->second.q_reader;

11

      assert (qr != nullptr);

12

13

      latency_map* const map = qr->getLatencyMap();

14

      assert (map != nullptr);

15

16

      for (it = map->begin(); it != map->end(); ++it) {

17

         const uint32_t ts = it->first;

18

         const time_t  tv = (time_t)ts;

19

         struct tm  my_tm;

20

         const  struct tm *result;

21

         result = gmtime_r(&tv, &my_tm);

22

         char     timeStr[32];

23

         char     dateStr[32];

24

25

         assert (result != nullptr);

26

         sprintf(dateStr, "%02d.%02d.%04d", my_tm.tm_mday, my_tm.tm_mon+1, my_tm.tm_year+1900);

27

         sprintf(timeStr, "%02d:%02d:%02d", my_tm.tm_hour, my_tm.tm_min, my_tm.tm_sec);

28

29

         o << (uint16_t) itm->first << ";";

30

         o << dateStr << " " << timeStr << ";";

31

32

         o << it->second << std::endl;

33

      }

34

   }

35

36

   return(o);

37

}

Jetzt Militärtauglich?

Tja, das hat der Author von QueueReader eine Beobachter-Funktion nicht 
als solche gekennzeichnet.

Die Namensgebung (und das ggf. vergessene const) lassen evtl. auf einen 
Java-Programmierer schließen.

Zudem können bei einem Zeiger zwei Sachen const sein: der 
Zeiger(variable) kann read-only sein und/oder das Ziel. Du hast also 
vier Möglichkeiten ...

Zeige mal die Schnittstelle von QueueReader.

1

class QueueReader

2

{

3

   public:

4

      QueueReader();

5

      virtual ~QueueReader();

6

7

      void  process (QueueData* q_data);

8

      bool           finish;

9

      inline latency_map *getLatencyMap(void) { return(&latency_map_); };

10

   protected:

11

12

   private:

13

      latency_map    latency_map_;

14

};

hmmm ....

Wilhelm M. schrieb:
> Zudem können bei einem Zeiger zwei Sachen const sein: der
> Zeiger(variable) kann read-only sein und/oder das Ziel. Du hast also
> vier Möglichkeiten ...

Ja, das weiss ich auch ;-). Wenn man in dem Code mal mit const beginnt, 
hat man ein halbes Jahr zu tun.

Warum ist die Beobachterfunktion non-const?

Und warum gibst Du einen Zeiger auf ein Datenmember als non-const-Ziel 
heraus? Dann brauchst Du es ja gar nicht private zu machen!

Also: hier fehlen ja die elementaren Grundkenntnisse ... Einführungskurs 
C++

René H. schrieb:
> Wenn man in dem Code mal mit const beginnt,
> hat man ein halbes Jahr zu tun.

Falsch. Man muss es von Anfang tun und das konsequent. Denn nur damit 
ergibt sich das "Gebäude", was sich const-correctness nennt.

Wahllos mit dem Salzstreuer const verteilen, ist kein Design.

René H. schrieb:
> inline latency_map *getLatencyMap(void) { return(&latency_map_);
> };

jetzt ist sie const, noch conster geht nicht :-)

1

      inline const latency_map *const getLatencyMap(void) { return(&latency_map_); };

Du hast leider den Unterschied zwischen einer Beobachterfunktion und 
Mutatorfunktion aka Modifier nicht verstanden (wahrscheinlich kommst Du 
aus der Java-Welt).

Das schaut sehr nach Anfänger aus. Die Aufgabe, mit der Du betraut bist, 
ist wohl eine Nummer zu groß für Dich im Moment.

Wilhelm M. schrieb:
> n Unterschied zwischen einer Beobachterfunktion und
> Mutatorfunktion aka M

Hab ich das?

Mein Studium liegt etwas zurück, und nein, ich bin kein Anfänger.

;-)

René H. schrieb:
> Wilhelm M. schrieb:
>> n Unterschied zwischen einer Beobachterfunktion und
>> Mutatorfunktion aka M
>
> Hab ich das?
>
> Mein Studium liegt etwas zurück, und nein, ich bin kein Anfänger.
>
> ;-)

PS: ich kenne die Design Patterns der Gang of Four. Also, was soll ich 
nicht begriffen haben?

PPS: Wie kommst Du eigentlich auf den Trichter das hier ein Observer 
Pattern zugrunde liegt?

Wilhelm M. schrieb:
> Falsch. Man muss es von Anfang tun und das konsequent. Denn nur damit
> ergibt sich das "Gebäude", was sich const-correctness nennt.

Das sagt sich bei einem großen Haufen Legacy-Code immer ganz besonders 
einfach.

Wilhelm M. schrieb:
> Das schaut sehr nach Anfänger aus. Die Aufgabe, mit der Du betraut bist,
> ist wohl eine Nummer zu groß für Dich im Moment.

Ich vermute mal sehr stark, dass René den Code nicht geschrieben hat und 
der auch nicht im luftleeren Raum schwebt. Wenn man da mit -Werror 
hantiert, dann fliegen einem relativ schnell auch andere Bestandteile um 
die Ohren.

Zumindest bei meinem Arbeitgeber wäre der Ansatz "gut, dann repariere 
ich den aufrufenden Code gleich mit" keine Alternative: Kein Mandat, 
kein Projekt, kein Reviewer, keine Zeit.

René H. schrieb:
> Also, was soll ich nicht begriffen haben?

Ich glaube, einfach nur die Welt der Theorie, in der Wilhelm lebt. :-)

Wilhelm M. schrieb:
> eine Nummer zu groß für Dich im Moment.

Tja, das mag sein, an dem wächst man. Noch habe ich immer mein Ziel 
erreicht und an dem habe ich nicht vor etwas zu ändern.

Schöner Code ist super, wenn man auf der schönen grünen Wiese beginnt. 
Das ist hier nicht der Fall.

Ich komme übrigens nicht aus der Java Welt sondern aus der Objective 
Pascal Welt.

Weshalb so überheblich? Du hast mir sehr viele gute Tipps gegeben die 
ich sehr zu schätzen weiss.

René H. schrieb:
> René H. schrieb:
>> Wilhelm M. schrieb:
>>> n Unterschied zwischen einer Beobachterfunktion und
>>> Mutatorfunktion aka M
>>
>> Hab ich das?
>>
>> Mein Studium liegt etwas zurück, und nein, ich bin kein Anfänger.
>>
>> ;-)
>
> PS: ich kenne die Design Patterns der Gang of Four. Also, was soll ich
> nicht begriffen haben?
>
> PPS: Wie kommst Du eigentlich auf den Trichter das hier ein Observer
> Pattern zugrunde liegt?

Davon habe ich nichts geschrieben!
Deine Funktion ist immer noch non-const.

S. R. schrieb:
> Ich glaube, einfach nur die Welt der Theorie, in der Wilhelm lebt. :-)

Danke! Ich habe schon begonnen an mir zu zweifeln ;-)

Man kann alles immer besser machen und ein Code-Review von aussen ist 
nie schlecht.

Der Code stammt in der Tat nicht von mir, ich mache nur Maintenance.

Wilhelm M. schrieb:
> Davon habe ich nichts geschrieben!
> Deine Funktion ist immer noch non-const.

Ja, aber das ist nicht das Problem, erst mal Funktionalität. Dann kann 
man es sauber machen.

Wilhelm M. schrieb:
> Davon habe ich nichts geschrieben!

Wilhelm M. schrieb:
> Du hast leider den Unterschied zwischen einer Beobachterfunktion und


;-)

wie gesagt, wenn ich das mit dem const durchziehe, bin ich ein halbes 
Jahr beschäftigt (und das bezahlt kein Mensch für "sauber").

René H. schrieb:
> Wilhelm M. schrieb:
>> Davon habe ich nichts geschrieben!
>
> Wilhelm M. schrieb:
>> Du hast leider den Unterschied zwischen einer Beobachterfunktion und
>
>
> ;-)
>
> wie gesagt, wenn ich das mit dem const durchziehe, bin ich ein halbes
> Jahr beschäftigt (und das bezahlt kein Mensch für "sauber").

Es geht doch nicht um "sauber", sondern um richtig, jetzt und v.a. in 
Zukunft. Rechne Deine Fehlersuchzeit dazu.

Btw. Const gab es schon vor c++98, und daher ist dieser Code wohl von 
Anfang an konzeptionell falsch. Auch wenn er ggf. Irgendwie läuft. Ist 
halt die Frage, was beim nä refactoring passiert.

Das sieht für mich nicht vertrauenserweckend aus. Heavy smell.

S. R. schrieb:
> Ich glaube, einfach nur die Welt der Theorie, in der Wilhelm lebt. :-)

Sorry, das sind Anfängerthemen.

Wilhelm M. schrieb:
> ht vertrauenserweckend aus. Heavy smell.

Der Code ist >20 Jahre alt und hat ein paar Millionen COL. Sowas baut 
man nicht einfach mal schnell neu (daran haben sich schon viele die 
Zähne ausgebissen und wurden schlussendlich gekündigt).

Es ist nicht das gelbe vom Ei, ist jetzt einfach so. Let's face it, das 
ist eher der Standard als unüblich.

Wilhelm M. schrieb:
> Sorry, das sind Anfängerthemen.

Nein, sind es nicht!

Wilhelm M. schrieb:
> Es geht doch nicht um "sauber", sondern um richtig,

Richtig ist, wenn es das macht was es muss. Der Rest ist sauber!

René H. schrieb:
> Wilhelm M. schrieb:
>> Sorry, das sind Anfängerthemen.
>
> Nein, sind es nicht!

Die Themen schon.
Leider nicht umgesetzt.

Wünsche Dir viel Glück mit dem Gebilde.

Wilhelm M. schrieb:
> Die Themen schon.
> Leider nicht umgesetzt.

Stimmt. ;-)

Wilhelm M. schrieb:
> Es geht doch nicht um "sauber", sondern um richtig, jetzt und v.a. in
> Zukunft. Rechne Deine Fehlersuchzeit dazu.
>
> Btw. Const gab es schon vor c++98, und daher ist dieser Code wohl von
> Anfang an konzeptionell falsch. Auch wenn er ggf. Irgendwie läuft. Ist
> halt die Frage, was beim nä refactoring passiert.
>
> Das sieht für mich nicht vertrauenserweckend aus. Heavy smell.

Naja, solange Du noch Geld an den Bankomaten bekommst .... ;-)

PS: Refactoring? Wovon träumst Du sonst noch so ....

PPS: der Code war ursprünglich C-Code. Dann machte man um die Headers 
noch ein Class drum und dann war es C++.

Wilhelm M. schrieb:
>> Ich glaube, einfach nur die Welt der Theorie, in der Wilhelm lebt. :-)
> Sorry, das sind Anfängerthemen.

Das mag sein, aber es ist für die Realität unerheblich.

Wenn das ursprüngliche Design von einem Anfänger gemacht wurde (oder auf 
einem antiken C++-Standard mit damals semi-kaputtem Compiler basiert), 
dann hilft so eine Aussage eben nicht.

René H. schrieb:
> PS: Refactoring? Wovon träumst Du sonst noch so ....
>
> PPS: der Code war ursprünglich C-Code. Dann machte man um die Headers
> noch ein Class drum und dann war es C++.

Beide Aussagen wirken aus meiner beruflichen Erfahrung... vollkommen 
korrekt. Danke. :-)

René H. schrieb:
> Wilhelm M. schrieb:
>> Es geht doch nicht um "sauber", sondern um richtig, jetzt und v.a. in
>> Zukunft. Rechne Deine Fehlersuchzeit dazu.
>>
>> Btw. Const gab es schon vor c++98, und daher ist dieser Code wohl von
>> Anfang an konzeptionell falsch. Auch wenn er ggf. Irgendwie läuft. Ist
>> halt die Frage, was beim nä refactoring passiert.
>>
>> Das sieht für mich nicht vertrauenserweckend aus. Heavy smell.
>
> Naja, solange Du noch Geld an den Bankomaten bekommst .... ;-)
>
> PS: Refactoring? Wovon träumst Du sonst noch so ....
>
> PPS: der Code war ursprünglich C-Code. Dann machte man um die Headers
> noch ein Class drum und dann war es C++.

Naja, jetzt verrennst du dich aber etwas. Du bist selbst nicht davon 
überzeugt, dass der Code so sein sollte, reproduzierst aber eben diese 
Qualität. Entweder du sagst "Sch**ß auf const. Ist nur Tipparbeit ohne 
jeden Nutzen" oder du solltest wenigstens die Sachen, die du neu 
schreibst, dem eigenen Maßstab folgend umsetzen.

Was Wilhelm mit const meint, ist, dass eine Funktion

1

struct X {

2

  int fn() const;

3

};

(grob gesagt) die Zusicherung trifft, das X nicht zu verändern. Also 
eine "nur lesende" oder beobachtende Funktion zu sein.
Weiter ausführend und den Begriff erweiternd, sollten Funktionen, die in 
der Weise "const" gekennzeichnet sind, von mehreren Threads parallel 
aufgerufen werden können, ohne dass Data-Races auftreten. Das dehnt den 
Begriff in der Weise aus, dass auch keine unsynchronisierten 
Seiteneffekte auftreten dürfen.

Problematisch im konkreten Fall ist, dass die Funktion selbst wieder ein 
komplexes Objekt zurückliefert (die map). Der Devise folgend, dass 
"const" genau die lesenden Funktionen markiert, die problemlos in 
nebenläufigen Kontexten verwendet werden können, sollte eine solche 
const-Funktion ein "const" Objekt zurückliefern. Also

1

const map& fn() const;

Da man aber ggf. in single-threaded Kontexten die map modifizieren will, 
braucht man 2 overloads: einmal const einmal non-const

1

const map& fn() const;

2

map& fn();

Die Folge einer solchen "const-correctness" ist z.B., dass man anhand 
der Deklarationen im Programm sofort sieht, wo Race-Conditions (nicht) 
auftreten können. Ist aber einiges an Tipparbeit :)

Heiko L. schrieb:
> Naja, jetzt verrennst du dich aber etwas. Du bist selbst nicht davon
> überzeugt, dass der Code so sein sollte, reproduzierst aber eben diese
> Qualität.

Das stimmt :-)

Heiko L. schrieb:
> oder du solltest wenigstens die Sachen, die du neu
> schreibst, dem eigenen Maßstab folgend umsetzen.

Sollte ich mal etwas neu Schreiben, halte ich bin daran. Das kannst Du 
mir glauben.

Grüsse,
René

Wie Wilhelm schon angemerkt hat, const-correctness muss schon ab dem 
Fundament gegeben sein. Damit kann man nicht nach >15 Jahren beginnen.
Es ist einfach nicht!

Grüsse
René

PS: Die Implementierung einer Klasse sollte eine BlackBox sein, wann 
hast Du das letzte mal eine Klasse gesehen die man so verwenden kann?

S. R. schrieb:
> Beide Aussagen wirken aus meiner beruflichen Erfahrung... vollkommen
> korrekt. Danke. :-)

Gott sei Dank, ich dachte schon wir sind die einzige Frickel-Bude :-).

@Mods

Sorry, aber das Thema passt jetzt nun wirklich nicht in dieses 
Unterforum.

So, das Ganze läuft jetzt ohne SEGV durch, auch über Stunden ohne Memory 
Leaks. Es waren die fehlenden returns (nicht das gmtime welches nicht 
Threadsafe ist).

Ich danke allen die mir geholfen haben.

Grüsse,
René

René H. schrieb:
> So, das Ganze läuft jetzt ohne SEGV durch, auch über Stunden ohne Memory
> Leaks. Es waren die fehlenden returns

Na, das freut mich dann doch ;-)

René H. schrieb:
> (nicht das gmtime welches nicht
> Threadsafe ist).

Das war ja auch nur in Bezug auf Qualität und auf Deinen Begriff der 
"militärtauglichkeit".

Trotzdem ist und bleibt das ein wichtiger Punkt. Auch Du solltest 
spätestens jetzt anfangen, Deinen Code in hoher Qualität schreiben zu 
wollen. Ein "das mache ich später" heißt ja in unserem Metier "nie".

Das bedeutet mindestens:

- Fehlerprüfung
- Zusicherungen
- Erweiterbarkeit
- Kapselung und Kohäsion

Und in diesem Zusammenhang ist gmtime() und / oder localtime() und 
andere vergleichbare Sünden der C-Bibliothek oder POSIX ein no-go.