NULL Pointer als Übergabe in Funktion verursacht sporadische Fehler

[Ralf (Gast)]

Hallo,
wenn ich folgende Funktion des öfteren aufrufe, kommt es zu sporadischen 
Fehlern im Programmablauf. Der Speicher verändert sich an gewissen 
Stellen.
Es liegt an der Übergabe des NULL-Zeigers, aber warum? Es ist eine 32Bit 
Architektur von Atmel (sam d21)

PS_ProgrammEvent(EVENT_PUSHNACHRICHT, NULL, 0, NULL);

Header:

void PS_ProgrammEvent(Event_Typ Event, uint8_t *Param, uint16_t len, uint8_t *id);

Funktion:

void PS_ProgrammEvent(Event_Typ Event, uint8_t *Param, uint16_t len, uint8_t *id)
{
  uint8_t LenByte;
  Programm_Typ Prog[2];  
  bool Setzen[2];          // Standardmäßig löschen der Aktivierung des Programms.

  memset(&Prog[0], 0, sizeof(Prog[0]));
  
  // Prüfen auf Verträglichkeit
  if(len > sizeof(InfoTable[0].Param))
    return;

  // Prüft dein Event gegen und schaltet die Programme um, falls nötig.
  switch(Event)
  {  
    case EVENT_EINGEHENDER_ANRUF:

      // Ein Anruf geht gerade ein.
      Prog[0] = PROG_EINGEHENDER_ANRUF;
      Setzen[0] = 1;    
                        
    break;
    case EVENT_EINGEHENDER_ANRUF_BEENDET:

      // Eingehender Anruf ist beendet.      
      Prog[0] = PROG_EINGEHENDER_ANRUF;              
      Setzen[0] = 0;  

    break;
    case EVENT_VERPASSTER_ANRUF:
    
      Prog[0] = PROG_VERPASSTER_ANRUF;
      Setzen[0] = 1;
    
    break;
    case EVENT_VERPASSTER_ANRUF_GELESEN:
    
      Prog[0] = PROG_VERPASSTER_ANRUF;
      Setzen[0] = 0;

    break;
    case EVENT_VERPASSTE_PUSHNACHRICHT:
    
      Prog[0] = PROG_VERPASSTE_NACHRICHT;
      Setzen[0] = 1;
    
    break;
    case EVENT_PUSHNACHRICHT_GELESEN:  
      
      Prog[0] = PROG_VERPASSTE_NACHRICHT;
      Setzen[0] = 0;    

    break;
    case EVENT_PUSHNACHRICHT:  
        
      Prog[0] = PROG_EINGEHENDE_NACHRICHT;

    break;
    case EVENT_BLE_VERBUNDEN:

      Prog[0] = PROG_BLE_CONNECTED;
      Setzen[0] = 1;

      Prog[1] = PROG_BLE_DISCONNECTED;
      Setzen[1] = 0;

    break;
    case EVENT_BLE_GETRENNT:

      Prog[0] = PROG_BLE_CONNECTED;
      Setzen[0] = 0;

      Prog[1] = PROG_BLE_DISCONNECTED;
      Setzen[1] = 1;

    break;
  }    

  // Gehe alle Vorhaben durch:
  for(uint8_t b = 0; b < sizeof(Prog)/sizeof(Prog[0]); b++)
  {  
    if(Prog[b] == PROG_UNDEFINIERT)
      continue;
    
    // Gehe alle Programme durch.
    for(uint8_t a = 0; a < SYS_AnzahlZustaende; a++)
    {
      // Gibt es ein Programm, dass aktiviert werden kann?
      if(InfoTable[a].ProgTyp == Prog[b])
      {
        switch(InfoTable[a].ProgTyp)
        {
          // Programmtyp passt.
          case PROG_EINGEHENDER_ANRUF:
            // TODO: Prüfe die Parameter des Programms.
                    
            InfoTable[a].Aktiviert = Setzen[b];  
                                        
          break;
          case PROG_VERPASSTE_NACHRICHT:  
                  
            // Prüfe den Parameter (App-ID)
            if(Setzen[b])
            {
              // Prüfe den Parameter:
              LenByte = InfoTable[a].Param[0];

              if(!LenByte || LenByte <= PS_ProgrammInfoParaLaenge && len == LenByte && !memcmp(&InfoTable[a].Param[1], Param, LenByte))
              {
                // Speicher die Paket ID ab:
                memcpy(&InfoTable[a].NotiID[0], id, sizeof(InfoTable[a].NotiID));
                InfoTable[a].Aktiviert = Setzen[b];                
              }                        
            }
            else if(!Setzen[b] && !memcmp(&InfoTable[a].NotiID[0], id, sizeof(InfoTable[a].NotiID)))
            {  
              InfoTable[a].Aktiviert = Setzen[b];              
            } 

          break;
          case PROG_VERPASSTER_ANRUF:
            // Verpasster Anruf:
            if(Setzen[b])
            {
              // Länge des Namens:
              LenByte = InfoTable[a].Param[0];

              // Wenn die Länge des hinterlegten Namens kleiner ist als die des Parameters.
              if(LenByte < len)              
                len = LenByte;              

              if(!LenByte || LenByte <= (PS_ProgrammInfoParaLaenge - 1) && len == LenByte && !memcmp(&InfoTable[a].Param[1], Param, LenByte))
              {
                // Speicher die Paket ID ab:
                memcpy(&InfoTable[a].NotiID[0], id, sizeof(InfoTable[a].NotiID));
                InfoTable[a].Aktiviert = Setzen[b];
              }
            }
            else if(!Setzen[b] && !memcmp(&InfoTable[a].NotiID[0], id, sizeof(InfoTable[a].NotiID)))
            {
              InfoTable[a].Aktiviert = Setzen[b];
            }
          break;
          case PROG_EINGEHENDE_NACHRICHT:

            // Übergangsprogramm einer eingehenden Nachricht:
            InfoTable[a].Aktiviert = 1;

          break;
          case PROG_BLE_CONNECTED:

            InfoTable[a].Aktiviert = Setzen[b];

          break;
          case PROG_BLE_DISCONNECTED:

            InfoTable[a].Aktiviert = Setzen[b];

          break;
        }
      }
    }
  }
  
  PS_ProgrammeUebernehmen();
}

[R. R. (elec-lisper)]

Also du iterierst über a und b, und nimmst die als Index für "Setzen", 
welches immer nur die Länge 2 hat. Dann liest du definitiv durchaus mal 
zufällig kram vom Stack/Speicher.
Das selbe passiert mit "id" und "Param", werden gelesen, obwohl sie ggf. 
NULL sind.
Der Code hat mir zu wenig defensive Checks eingebaut.
Evtl. würde es auch helfen, wenn du die Routine etwas aufspaltest in 
verschiedene Funktionen - für die Verständlichkeit, wobei die an sich
gut ist mit den Variablennamen und Kommentaren. Auch wenn mir noch
zuviele Unbekannte da sind, bspw. wie groß InfoTable wirklich ist,
und warum es abhängig ist von SYS_AnzahlZustaende, müsste sie dann nicht
StateTable oder ZustandsTable heißen?

[Rufus Τ. F. (rufus)]

Ralf schrieb:
> Es liegt an der Übergabe des NULL-Zeigers, aber warum?

Kann nicht sein. Die Funktion wertet ihren zweiten Parameter nirgends 
aus.

[Peter II (Gast)]

der code ist nicht gerade gut zu lesen, viel zu verschachtelt.

im ersten Moment kann ich aber keinen Fehler finden, kann es sein das es 
in  PS_ProgrammeUebernehmen() zu einen Fehler kommt.

[Theor (Gast)]

Rufus Τ. F. schrieb:
> Ralf schrieb:
>> Es liegt an der Übergabe des NULL-Zeigers, aber warum?
>
> Kann nicht sein. Die Funktion wertet ihren zweiten Parameter nirgends
> aus.

Leider doch. An zwei Stellen. Aber ich musste auch erst suchen.

[R. R. (elec-lisper)]

Was mir auch noch auffällt:

if(Prog[b] == PROG_UNDEFINIERT)

Du setzt nirgends PROG_UNDEFINIERT. Wozu ist dann
der Check da? Du kannst nicht annehmen, dass deine
Strukturen und Variablen auf dem Stack mit 0 initialisiert
sind! Du setzt ja nur Prog[0] auf 0 mit:

memset(&Prog[0], 0, sizeof(Prog[0]));

[Ralf (Gast)]

Robin R. schrieb:
> Das selbe passiert mit "id" und "Param", werden gelesen, obwohl sie ggf.
> NULL sind.

Nö werden sie von dem Funktionsaufruf nicht. Wie willst du einen Pointer 
denn auf Länge prüfen?

Rufus Τ. F. schrieb:
> Kann nicht sein. Die Funktion wertet ihren zweiten Parameter nirgends
> aus.

Eben aber wenn ich den Funktionsaufruf noppe funktioniert alles Stabil.
Nur dieser Aufruf verursacht den Fehler nach einigen Aufrufen.

Peter II schrieb:
> im ersten Moment kann ich aber keinen Fehler finden, kann es sein das es
> in  PS_ProgrammeUebernehmen() zu einen Fehler kommt.

Passiert nichts dolles:

void PS_ProgrammeUebernehmen(void)
{
  // Übernimmt das aktivierte Programm mit der höchsten Priorität:
  bool KeinProgrammGefunden = 1;

  // Prüfe auf Umschalten - Grundprogramm wird ignoriert:
  for(uint8_t a = 1; a < SYS_AnzahlZustaende; a++)
  {
    if(InfoTable[a].Aktiviert)
    {
      if(LS_AktuellesProgramm != a)
      {
        LS_AktuellesProgramm = a;
        LS_ProgrammNeuLaden = 1;
      }
      
      KeinProgrammGefunden = 0;
      break;
    }
  }

  // Wenn es keine aktivierten Programme gibt, schalte in das 0. Programm!
  if(KeinProgrammGefunden && LS_AktuellesProgramm != 0)
  {
    LS_AktuellesProgramm = 0;
    LS_ProgrammNeuLaden = 1;
  }
}

[Ralf (Gast)]

Robin R. schrieb:
> Du setzt nirgends PROG_UNDEFINIERT.

Doch:

typedef enum{
  EVENT_EINGEHENDER_ANRUF,
  EVENT_EINGEHENDER_ANRUF_BEENDET,

  EVENT_VERPASSTER_ANRUF,
  EVENT_VERPASSTER_ANRUF_GELESEN,

  EVENT_VERPASSTE_PUSHNACHRICHT,
  EVENT_PUSHNACHRICHT_GELESEN,

  EVENT_BLE_VERBUNDEN,
  EVENT_BLE_GETRENNT,

  // Blending Programme (Übergangsprogramme):
  EVENT_PUSHNACHRICHT,
} Event_Typ;

[Theor (Gast)]

@ Ralf

Andere hier reagieren vielleicht anders und mehr, wie Du es wünschst, 
aber ich muss leider sagen, dass ich mir diesen Code nicht antue.

Dafür gibt es abstrakte Gründe, die Dir vielleicht nützlich erscheinen, 
vielleicht aber auch nicht:

Zum einen hast Du sehr viel Code in eine Funktion geklemmt. Das ist 
nicht eigentlich falsch, führt aber dazu, dass sie nur mit hohem Aufwand 
zu testen ist. Ich rate Dir, die Funktion in mehrere kleinere 
aufzuteilen. Eine bewährte Daumenregel lautet: "Eine Aufgabe, eine 
Funktion" und eine weitere, ebenso bewährte: "Eine Funktion, eine 
Bildschirmseite" (also etwa 25 Zeilen à 80 Zeichen).

Zum anderen ist die Fehlerbeschreibung bzw. die Analyse zu 
oberflächlich.
Es fängt damit an, dass eine NULL als Parameter an sich niemals einen 
Fehler hervorruft, wenn man voraussetzt, dass als Parameter an sich ein 
Zeiger zulässig ist. Das wusstest Du vielleicht nicht.
Aber wenn man das voraussetzt, dann ruft eine NULL effektiv einen Fehler 
bei ihrer Verwendung in einem Ausdruck einen Fehler hervor. Die Stelle 
an der das geschieht musst Du finden und mit einer Wahrscheinlichkeit 
ist auch ziemlich offensichtlich, was da genau schiefläuft - leider 
nicht immer, oft eben auch nicht.

Also bitte überarbeite Deinen Code. Überlege Dir Tests für die 
Einzelfunktion und baue die dann in die Funktion PS_ProgrammEvent ein.

[R. R. (elec-lisper)]

Ralf schrieb:
> Robin R. schrieb:
>> Du setzt nirgends PROG_UNDEFINIERT.
>
> Doch:
>
>

> typedef enum{

Ähm? Ja? Und wo weist du PROG_UNDEFINIERT jemals zu?

> } Event_Typ;
>

[Theor (Gast)]

Ralf schrieb:
> Rufus Τ. F. schrieb:
>> Kann nicht sein. Die Funktion wertet ihren zweiten Parameter nirgends
>> aus.
>
> Eben ...

Das sehe ich anders:

In dieser Zeile:

if(!LenByte || LenByte <= PS_ProgrammInfoParaLaenge && len == LenByte && !memcmp(&InfoTable[a].Param[1], Param, LenByte))

und in dieser Zeile

if(!LenByte || LenByte <= (PS_ProgrammInfoParaLaenge - 1) && len == LenByte && !memcmp(&InfoTable[a].Param[1], Param, LenByte))

wird Param verwendet.

[Ralf (Gast)]

Ralf schrieb:
> Du setzt nirgends PROG_UNDEFINIERT.

Sry. Meinte das hier:

typedef enum{
  // Stabile Programme (Hauptprogramme)
  PROG_UNDEFINIERT = 0,                            // Undefiniertes Programm
  PROG_VERPASSTER_ANRUF = 1,                          // Nicht angenommener Anruf
  PROG_VERPASSTE_NACHRICHT = 2,                        // Ungelesene Nachricht
  PROG_EINGEHENDER_ANRUF = 3,                          // Eingehender Anruf
  PROG_BLE_CONNECTED = 4,                            // iPhone per Bluetooth verbunden
  PROG_BLE_DISCONNECTED = 5,

  // Blending Programme (Übergangsprogramme)
  PROG_EINGEHENDE_NACHRICHT = 254,
} Programm_Typ;

[R. R. (elec-lisper)]

Ralf schrieb:
> Robin R. schrieb:
>> Das selbe passiert mit "id" und "Param", werden gelesen, obwohl sie ggf.
>> NULL sind.
>
> Nö werden sie von dem Funktionsaufruf nicht. Wie willst du einen Pointer
> denn auf Länge prüfen?

Was und ob du auf id oder Param zugreifst hängt allein vom Inhalt
in InfoTable[a] ab. Außerdem verstehe ich nicht was du mit Länge meinst.
Ein NULL-Pointer zeigt auf 0x0, und Lesezugriffe davon sind schonmal
bedenklich.

Ehrlich, bau mehr Sicherheitsprüfungen ein und initialisiere deine
Variablen richtig. Dann schau ob die Fehler weg gehen.

[Ralf (Gast)]

Theor schrieb:
> wird Param verwendet.

Ja aber nicht bei dem konkreten Aufruf.

[R. R. (elec-lisper)]

Ralf schrieb:
> Theor schrieb:
>> wird Param verwendet.
>
> Ja aber nicht bei dem konkreten Aufruf.

Können wir nicht ableiten aus dem unvollständigen
Code und Programmzustand.
Und wenn du genau weisst, was dein Code tut,
dann weisst du auch wo der Fehler ist.

[Nop (Gast)]

Eine zweifach verschachtelte Schleife mit einer mehrdimensionalen 
Statemachine in der inneren Schleife? WTF? Soll das später noch 
irgendwer anderes warten, oder ist das bloß Wegwerfcode zum privaten 
Eigengebrauch?

[Ralf (Gast)]

Nop schrieb:
> Eine zweifach verschachtelte Schleife mit einer mehrdimensionalen
> Statemachine in der inneren Schleife? WTF? Soll das später noch
> irgendwer anderes warten, oder ist das bloß Wegwerfcode zum privaten
> Eigengebrauch?

Nö, für die Tonne.

[Theor (Gast)]

Ralf schrieb:
> Theor schrieb:
>> wird Param verwendet.
>
> Ja aber nicht bei dem konkreten Aufruf.

Richtig. Wenn Du den oben zitierten Aufruf machst, dann nicht.
Dann wird aber auch nichts weiter in der ganzen Funktion getan, als

InfoTable[a].Aktiviert = 1;

was in der später geposteten Funktion PS_ProgrammeUebernehmen zunächst 
zu nichts weiter als setzen von zur Compile-Zeit definierten Variablen 
bewirkt.

Darin liegt der Fehler jedenfalls nicht.


Ich verweise nochmal auf meinen Beitrag hier: 
Beitrag "Re: NULL Pointer als Übergabe in Funktion verursacht sporadische Fehler"

[Peter D. (peda)]

Ralf schrieb:
> void PS_ProgrammEvent(Event_Typ Event, uint8_t *Param, uint16_t len,
...
>   if(len > sizeof(InfoTable[0].Param))

Du hast also einen Bytepointer *Param und eine Struct mit einem Member 
.Param.
Ist zwar erlaubt, sollte man aber nicht tun, da es sehr verwirrend ist.
Nimm für verschiedene Sachen auch verschiedene Namen.

Wenn Du explizit für Parameter NULL zuläßt, solltest Du sie auch 
explizit auf NULL testen.

Man kann überlange Zeilen auch sinnvoll umbrechen, C ist nicht 
zeilensensitiv.

Das ganze sieht wahnsinnig verworren aus, erweckt aber den Eindruck, daß 
man es viel einfacher und klarer schreiben könnte.

[Theor (Gast)]

Dachte das wäre zu unhöflich, aber ich möchte Peter zustimmen: Sehr 
wuselig das Ganze.

[Ralf (Gast)]

Peter D. schrieb:
> Du hast also einen Bytepointer *Param und eine Struct mit einem Member
> .Param.
> Ist zwar erlaubt, sollte man aber nicht tun, da es sehr verwirrend ist.
> Nimm für verschiedene Sachen auch verschiedene Namen.

Guter Anreiz, werde ich ändern!

Peter D. schrieb:
> Das ganze sieht wahnsinnig verworren aus, erweckt aber den Eindruck, daß
> man es viel einfacher und klarer schreiben könnte.

Ist es auch, ist eben eine Abarbeitung von Zuständen. Wenn ich in der 
switch alles in Funktionen Packe, trägt das der Übersichtlichkeit bei, 
das stimmt schon, mache ich ggf. auch.

Grade probiert:

PS_ProgrammEvent2(EVENT_PUSHNACHRICHT, NULL, 0, NULL);

void PS_ProgrammEvent2(Event_Typ Event, uint8_t *Param, uint16_t len, uint8_t *id)
{

}

[c]void PS_ProgrammEvent2(Event_Typ Event, uint8_t *Param, uint16_t len, 
uint8_t *id);[c]

Verursacht ähnliche Probleme.

[Theor (Gast)]

Ralf schrieb:
> Grade probiert:
>
>

PS_ProgrammEvent2(EVENT_PUSHNACHRICHT, NULL, 0, NULL);

>
>

void PS_ProgrammEvent2(Event_Typ Event, uint8_t *Param, uint16_t len, 
> uint8_t *id)
> {
> 
> }

>
> [c]void PS_ProgrammEvent2(Event_Typ Event, uint8_t *Param, uint16_t len,
> uint8_t *id);[c]
>
> Verursacht ähnliche Probleme.

Schön. Aber was sollen wir nun damit anfangen?
Du musst das Problem erstmal genau feststellen, beschreiben worin es 
besteht. Ich wiederhole: Eine Null als Parameter verursacht an sich 
keinen Fehler.

[Theor (Gast)]

Ich glaub, ich gehe ins Bett und lese Don Quichotte. Das passt genau zum 
Thread. :-)

[Robin.R. (Gast)]

Und ich nehm an meine vielfältigen Bemerkungen werden nicht verstanden?
Schreib ich überhaupt Deutsch? Oder schlaf ich schon?

[(prx) A. K. (prx)]

Sollte das
  memset(&Prog[0], 0, sizeof(Prog[0]));
nicht eher
  memset(Prog, 0, sizeof(Prog));
sein?

[Ralf (Gast)]

A. K. schrieb:
> Sollte das
>   memset(&Prog[0], 0, sizeof(Prog[0]));
> nicht eher
>   memset(Prog, 0, sizeof(Prog));
> sein?

Erster produktiver Beitrag, habe ich grade auch bemerkt.
Genau dadurch entstand der Fehler !

Danke

[Ralf (Gast)]

A. K. schrieb:
> memset(&Prog[0], 0, sizeof(Prog[0]));

memset(&Prog[0], 0, sizeof(Prog));

[Peter II (Gast)]

Ralf schrieb:
> memset(&Prog[0], 0, sizeof(Prog));

oder einfach gleich auf 0 setzen

Programm_Typ Prog[2] = {0};

[Ralf (Gast)]

Peter II schrieb:
> oder einfach gleich auf 0 setzen

Setzt das nicht nur den ersten der Beiden auf null?

[Ralf (Gast)]

Ralf schrieb:
> Setzt das nicht nur den ersten der Beiden auf null?

Ne, gute Idee!

[Peter II (Gast)]

Ralf schrieb:
> Setzt das nicht nur den ersten der Beiden auf null?

nein, setzt alles auf 0

[(prx) A. K. (prx)]

Ralf schrieb:
> memset(&Prog[0], 0, sizeof(Prog));

ist identisch mit
  memset(Prog, 0, sizeof(Prog));

a[i] ist *(a+i)
&a[0] ist &*(a+0) ist a

[Robin.R. (Gast)]

Ralf schrieb:
> Erster produktiver Beitrag, habe ich grade auch bemerkt.
> Genau dadurch entstand der Fehler !

Danke, genauauf die Stelle hab ich schon vor dutzenden Posts 
hingewiesen.
Liest denn keiner meine Posts oder muss man jetzt alles vorkauen?

[Theor (Gast)]

Robin.R. schrieb:
> Ralf schrieb:
>> Erster produktiver Beitrag, habe ich grade auch bemerkt.
>> Genau dadurch entstand der Fehler !
>
> Danke, genauauf die Stelle hab ich schon vor dutzenden Posts
> hingewiesen.
> Liest denn keiner meine Posts oder muss man jetzt alles vorkauen?

Tröste Dich. Da bist Du nicht der einzige hier.

[Robin.R. (Gast)]

@Theor: Danke ;-)

[Rafl (Gast)]

Robin.R. schrieb:
> Ralf schrieb:
> Erster produktiver Beitrag, habe ich grade auch bemerkt.
> Genau dadurch entstand der Fehler !
>
> Danke, genauauf die Stelle hab ich schon vor dutzenden Posts
> hingewiesen.
> Liest denn keiner meine Posts oder muss man jetzt alles vorkauen?

Stimmt, hatte ich überlesen, tut mir leid.

[Rafl (Gast)]

Können auch gerne mal über die Struktur diskutieren. Die Funktion 
verknüpft Events mit Zuständen, klar kann ich in der switch die Code 
Teile auslagern. Nur die erste switch Anweisung legt quasi die Zustände 
fest, die durch ein Event geändert werden müssen und Prog ist die 
"Warteschlange"