Forum: Compiler & IDEs übertragungsfehler


von blan (Gast)


Lesenswert?

hallo,

ich programmiere gerade an einer kommuniktion zwischen µC und meinem PC 
über rs232. als puffer nehm ich ein selbst programmierten FIFO. jetzt 
find ich den fehler einfach nicht. wenn mein code so aussieht (aus 
main-methode):
1
while(TRUE)
2
    {
3
      int8_t data;
4
5
      if(fifo_pop(fifo_rx, &data) == FIFO_SUCCESS)
6
      {
7
        uart_putc(data);
8
      }
9
    }

bekomm ich datenübertragungsfehler in form von zeichen die nicht im FIFO 
liegen. sieht mein code dagegen so aus, bekomm ich keine fehler:
1
while(TRUE)
2
    {
3
      int8_t *data = (int8_t*)malloc(1);
4
5
      if(fifo_pop(fifo_rx, data) == FIFO_SUCCESS)
6
      {
7
        uart_putc(*data);
8
      }
9
10
      free(data);
11
    }

mich würde interessiere wieso das so ist. vielleicht hat jemand von euch 
eine idee - ich komm nicht dahinter. beides sollte doch eigentlich das 
gleiche sein nur das einmal der speicher manuell allokiert wird oder?
1
int8_t fifo_pop(volatile const fifo_t *fifo, int8_t *data)
2
{
3
  if(fifo == NULLPOINTER)
4
  {
5
    return FIFO_ERROR;
6
  }
7
8
  if(data == NULLPOINTER)
9
  {
10
    return FIFO_ERROR;
11
  }
12
13
  if(strlen(fifo->data) >= fifo->fragment.size)
14
  {
15
    if(memcpy((int8_t*) data, (int8_t*) fifo->data, fifo->fragment.size))
16
    {
17
      if(memcpy((int8_t*) fifo->data, (int8_t*) fifo->data + fifo->fragment.size, strlen(fifo->data) - fifo->fragment.size))
18
      {
19
        if(memset((int8_t*) fifo->data + (strlen(fifo->data) - fifo->fragment.size), NULLBYTE, fifo->size - strlen(fifo->data) - fifo->fragment.size))
20
        {
21
          return FIFO_SUCCESS;
22
        }
23
        else
24
        {
25
          return FIFO_ERROR;
26
        }
27
      }
28
      else
29
      {
30
        return FIFO_ERROR;
31
      }
32
    }
33
    else
34
    {
35
      return FIFO_ERROR;
36
    }
37
  }
38
  else
39
  {
40
    return FIFO_ERROR;
41
  }
42
}
1
void uart_putc(const int8_t c)
2
{
3
  // Wartet, bis UDR bereit zum senden ist
4
  while (!(UCSRA & (1 << UDRE)));
5
6
  UDR = c;
7
}

mfg blan

von Stefan B. (stefan) Benutzerseite


Lesenswert?

Du hast Platz für ein Zeichen

      int8_t data;

und machst möglicherweise dieses:

  if(strlen(fifo->data) >= fifo->fragment.size)
  {
    if(memcpy((int8_t*) data, (int8_t*) fifo->data, 
fifo->fragment.size))

Der potentielle Bufferoverflow in data hängt natürlich davon ab, was in 
fifo steht (den Code zeigst du nicht).

Bei dem zweiten Verfahren mit malloc() fällt der gleiche Buffreoverflow 
möglicherweise nicht sofort auf. Bei dem ersten Verfahren knallt es 
hingegen sofort, weil data auf dem Stack liegt und du Rücksprungadressen 
etc. zerstörst.

von blan (Gast)


Lesenswert?

bufferoverflow - jetzt bin ich bischen unsicher :-/

also hier mal der ganze code:

fifo.c: http://nopaste.debianforum.de/6095
fifo.h: http://nopaste.debianforum.de/6096

wäre schon wenn du mir den fehler zeigen könntest - danke

mfg blan

von Florian (Gast)


Lesenswert?

Hier geht schon einmal das Null-Byte verloren:
 if(memcpy((int8_t*) fifo->data + strlen(fifo->data), (int8_t*) data, 
strlen(data))

und das hier:
 if(memcpy((int8_t*) data, (int8_t*) fifo->data, fifo->fragment.size))

macht böse Dinge, wenn fragment.size > 1 ist.

von blan (Gast)


Lesenswert?

warum geht da das Nullbyte verloren. ich erstell doch extra bei 
fifo_new() ein byte mehr das nicht überschrieben wird und 0 ist?

das es böse dinge macht wenn fragment.size > 1 ist mir schon klar, dann 
würd ich aber auch ein char-array mit der entsprechenden größe nehmen..

mfg blan

von Florian (Gast)


Lesenswert?

Das mit dem Nullbyte habe ich falsch gesehen. Du initialisierst ja 
vorher alles auf 0.

memcpy((int8_t*) fifo->data, (int8_t*) fifo->data + fifo->fragment.size, 
strlen(fifo->data) - fifo->fragment.size)

memset((int8_t*) fifo->data + (strlen(fifo->data) - 
fifo->fragment.size), NULLBYTE, fifo->size - strlen(fifo->data) - 
fifo->fragment.size)

würde ich schon einmal in

memmove( fifo->data, fifo->data + fifof->fragment.size, strlen( 
fifo->data ) - fifo->fragment.size + 1 );

umwandeln.

Das verschiebt dann auch das Null-Byte mit. Außerdem memmove anstatt 
memcpy, da sich die Bereiche überlappen:

MEMCPY(3) 
Bibliotheksfunktionen 
MEMCPY(3)



BEZEICHNUNG
       memcpy - kopiere Speicherbereich

UBERSICHT
       #include <string.h>

       void *memcpy(void *dest, const void *src, size_t n);

BESCHREIBUNG
       Die  Funktion  memcpy()  kopiert  n  Bytes  von Speicherbereich 
src nach Speicherbereich dest.  Die Speicherbereiche durfen sich nicht
       uberschneiden.  Benutze memmove(3), wenn sich die 
Speicherbereiche uberschneiden..

RUCKGABEWERT
       Die Funktion memcpy() gibt einen Zeiger auf dest zuruck.

KONFORM ZU
       SVID 3, BSD 4.3, ISO 9899

SIEHE AUCH
       bcopy(3), memccpy(3), memmove(3), strcpy(3), strncpy(3).

von blan (Gast)


Lesenswert?

also mein fifo-buffer scheint wohl doch nicht so gut zu sein wie ich 
dachte :-/

wie meinst du das mit dem überlappen und der memmove-funktion? findet 
der buffer-overflow jetzt eigentlich statt und liegt es nur an diesem 
überlappen?

mfg blan

von blan (Gast)


Lesenswert?

also ich glaub ich hab jetzt verstanden was du meinst. ich hab den code 
abgeändert und mal in ein testprogramm zusammengefasst und dort 
funktioniert er scheinbar einwandfrei. ich hab ihn hier mal hochgeladen:

http://nopaste.debianforum.de/6098

als ausgabe bekomm ich folgendes:
1
104
2
97
3
108
4
108
5
111
6
97
7
98
8
99
9
0
10
----------------------------
11
h
12
----------------------------
13
97
14
108
15
108
16
111
17
97
18
98
19
99
20
0
21
0

sieht doch ganz gut aus oder?

mfg blan

von Florian (Gast)


Lesenswert?

Mit dem Überlappen:

memcpy((int8_t*) fifo->data, (int8_t*) fifo->data + fifo->fragment.size,
strlen(fifo->data) - fifo->fragment.size)


Hier besteht der Bereich aus

Ziel:
fifo->data bis fifo->data + strlen( fifo->data ) - fifo->fragment.size 
-1

Quelle:
fifo->data + fifo->fragment.size bis fifo->data + strlen( fifo->data ) 
-1


Nehmen wir an:

fifo->data = "12345";
fifo->fragment.size = 1

-->
Ziel: fifo->data[0] bis fifo->data[3]
Quelle: fifo->data[1] bis fifo->data[4]

Die Bereiche sind also zum Teil identisch.

Es hängt von der genauen Implementation von memcpy ab, wie das 
funktioniert. Wenn auf bestimmten Platformen z.B. 32-Bit auf einmal 
kopiert werden (sofern Länge und Alignment das zulassen), würde hier 
schon murks herauskommen.

Hier ist es sicherer, memmove zu verwenden, da das effektiv genau dafür 
existiert.


Das
memset((int8_t*) fifo->data + (strlen(fifo->data) -
fifo->fragment.size), NULLBYTE, fifo->size - strlen(fifo->data) -
fifo->fragment.size)

setzt ja hinter jeden memcpy alle Bytes, die nachfolgen, auf 0. Da Du 
anfangs mit 0 initialisierst, könntest Du einfach die Länge des cpy bzw. 
move um 1 erhöhen (der Puffer ist ja ein Byte länger als nötig), und 
schon sparst Du Zeit.


Bsp:

data = { 'a', 'b', 'c', 0 }; // + etliche Nullen
nach Deinem cpy (wenn wir unterstellen, dass er geklappt hat):
data = { 'b', 'c', 'c', 0 };

hättest Du ein Byte mehr kopiert:

data = { 'b', 'c', 0, 0 }; // die letzte Null kommt aus dem riesigen 
Nullen-Pool

Übrigens die ganzen strlen könntest Du Dir sparen (wären auch 
hinderlich, sofern 0 ein zulässiger Wert ist), indem Du einen Zähler 
hinzufügst:

typedef struct {
   int pointer;
...} fifo_t;

strlen(fifo->data) jeweils durch pointer ersetzt und bei push

fifo->pointer += strlen(data);

sowie bei pop

fifo->pointer -= fifo->fragment.size;

benutzt.

Weiterhin frage mich, was das const bei pop zu suchen hat.

Funktioniert das nun denn auch in Deiner Zielanwendung?

von blan (Gast)


Lesenswert?

okay, dann hab ich das mit dem überlappen richtig verstanden und hab 
auch schon memmove eingebaut. der code funktioniert so wie vorher - es 
hat sich also nichts getan.

das mit dem pointer um strlen() zu ersetzen ist eine gute idee. nur muss 
ich erstmal wissen warum das ganze scheinbar ohne probleme in dem 
C-Programm funktioniert und nicht auf dem µC. ich seh in dem code 
keinerlei bufferoverflow oder sonstiges.

das const scheint da irgendwie reingerutscht zu sein :D

findest du nicht noch irgendwo ein fehler?

mfg blan

von Florian (Gast)


Lesenswert?

Kannst Du einmal aufschreiben, welche Zeichen genau ankommen und welche 
Du erwarten würdest?

Bist Du im Übrigen sicher, dass die RS232-Übertragung sauber 
funktioniert?

BTW: Du bekommst sicher Warnungen bzgl. NULLPOINTER. Verwende doch 
stattdessen einfach NULL.

von blan (Gast)


Lesenswert?

also es handelt sich um ein echo. das was ich sende sollte auch zurück 
kommen. wenn ich ein 0x61 (a) sende kommt zu 60% ein 0x61 (a) zurück und 
dazwischen immer wieder 0x04 0x5E (immer die beiden hintereinander).

ja die funktioniert, denn kaum änder ich den code (wie oben beschrieben) 
bekomm ich zu 100% 0x61 (a) zurück.

ich hab NULLPOINTER mit 0x00 definiert - ich bekomm keine warnung.

mfg blan

von Florian (Gast)


Lesenswert?

Das mal zum Thema Warnungen:

chef@localhost /tmp $ gcc -c -Wall fifo.c
fifo.c: In Funktion »main«:
fifo.c:62: Warnung: Zeigerziele bei Übergabe des Arguments 2 von 
»fifo_push« unterscheiden sich im Vorzeichenbesitz
fifo.c:88: Warnung: Kontrollfluss erreicht Ende einer 
Nicht-void-Funktion
fifo.c: In Funktion »fifo_push«:
fifo.c:140: Warnung: Zeigerziele bei Übergabe des Arguments 1 von 
»strlen« unterscheiden sich im Vorzeichenbesitz
fifo.c:140: Warnung: Zeigerziele bei Übergabe des Arguments 1 von 
»strlen« unterscheiden sich im Vorzeichenbesitz
fifo.c:146: Warnung: Zeigerziele bei Übergabe des Arguments 1 von 
»strlen« unterscheiden sich im Vorzeichenbesitz
fifo.c:146: Warnung: Zeigerziele bei Übergabe des Arguments 1 von 
»strlen« unterscheiden sich im Vorzeichenbesitz
fifo.c: In Funktion »fifo_pop«:
fifo.c:169: Warnung: Zeigerziele bei Übergabe des Arguments 1 von 
»strlen« unterscheiden sich im Vorzeichenbesitz
fifo.c:173: Warnung: Zeigerziele bei Übergabe des Arguments 1 von 
»strlen« unterscheiden sich im Vorzeichenbesitz

von Florian (Gast)


Angehängte Dateien:

Lesenswert?

Habe Deinen Quelltext überarbeitet und die Geschichte mit dem Zähler 
eingabaut; außerdem die tiefe Verschachtelung beseitigt.

Wenn Du
1
if ( a == b )
2
     return 1;
3
else
4
     return 2;

machst und dsa schachtelst, ist das eher nicht so gut lesbar. Das 'else' 
kannst Du Dir sparen, da es nach einem Return dort nicht weitergehen 
würde.

Der geänderte Code ist im Anhang und lässt sich ohne Warnungen 
kompilieren. Vielleicht klappt es ja hiermit.

von blan (Gast)


Lesenswert?

ich verzweifel noch. der code funtkioniert so wie meiner davor auch wenn 
ich die fragment-größe auf 4 stelle und es in ein array (int8_t data[4]) 
speichern lasse funktionierts nicht. sobald ich das array mit data = 
malloc(5); initalisiere funktioniert es einwandfrei.

ich hab noch fragen zu dem code:

- warum setzt du fifo->data beim erstellen nicht auf \0? - es 
erleichtert das zählen der aktuellen größe.

- warum machst du fifo->data nur so groß wie size und nicht eins größer 
für das nullbyte? - beim zählen der größe von fifo->data kommt es sonst 
zum bufferoverflow.

mfg blan

von Florian D. (code-wiz)


Lesenswert?

In meinem Code wird nicht mehr mit strlen gearbeitet, daher ist es 
überflüssig, das Array auf 0 zu setzen und das zusätzlich Byte ist 
ebenfalls nicht notwendig.

Vielleicht wäre jetzt der Zeitpunkt, zu erfahren, wie genau Du das 
fifo_push aufrufst bzw. wie der Rest des Codes aussieht.

von blan (Gast)


Lesenswert?

mir ist klar, dass der fifo-code nicht mehr mit strlen arbeitet. aber um 
herauszubekommen wie groß die aktuellen daten im fifo-buffer sind brauch 
ich eben strlen(). ich hab den fifo-code jetzt um ein memset(..., '\0', 
...) und ein nullbyte am ende erweitert um den code hier zu realisieren:

http://nopaste.debianforum.de/6101 (main.c)

mfg blan

von Florian D. (code-wiz)


Lesenswert?

>mir ist klar, dass der fifo-code nicht mehr mit strlen arbeitet. aber um
>herauszubekommen wie groß die aktuellen daten im fifo-buffer sind brauch
>ich eben strlen()

Dafür gibt es doch nun fifo->pointer.

> memset(..., '\0',...)

Das '\0' kannst Du auch einfach durch 0 ersetzen (ohne 
Anführungsstriche, ohne Backslash).


von Florian D. (code-wiz)


Lesenswert?

Das Einzige, was mit auf die Schnelle noch einfällt, ist, dass sich der 
memmove im pop und der memcpy im push gegenseitig stören. Du könntest 
testweise mal die Interrupts in der pop-Routine deaktivieren und danach 
wieder einschalten.

Sonst müsstest Du den FIFO wohl 'sauberer' aufbauen, also z.B. als 
Ringpuffer mit zwei Pointern (einer zum Lesen, einer zum Schreiben). Da 
sollten solche Effekte dann gänzlich verschwinden.

von blan (Gast)


Lesenswert?

stimmt, du hast recht - ich hab ja jetzt den pointer. jetzt ist mir aber 
aufgefallen, dass der code scheinbar nicht einwandfrei funktioniert. als 
ausgabe bekomm ich folgendes:
1
h
2
a
3
l
4
l
5
o
6
a
7
b
8
c
9
----------------------------
10
h
11
----------------------------
12
a
13
l
14
l
15
o
16
a
17
b
18
c
19
c

(man beachte die zwei "c") obwohl mit der memmove-funktion gearbeitet 
wird?

mfg blan

von blan (Gast)


Lesenswert?

also auch wenn ich den interrupt für die pop-methode deaktiviere und 
danach wieder aktiviere funktioniert es nur wenn ich den speicher mit 
malloc() allokiere. kann doch nicht sein o_0

mfg blan

von Florian D. (code-wiz)


Lesenswert?

> (man beachte die zwei "c") obwohl mit der memmove-funktion gearbeitet
> wird?

Ja, das ist schon richtig. Der Pointer zeigt den Füllstand an. Es gibt 
kein Null-Byte mehr dahinter.

von Florian D. (code-wiz)


Lesenswert?

blan wrote:
> also auch wenn ich den interrupt für die pop-methode deaktiviere und
> danach wieder aktiviere funktioniert es nur wenn ich den speicher mit
> malloc() allokiere. kann doch nicht sein o_0

Vielleicht ist das irgendein Seiteneffekt. Vielleicht funktioniert die 
RS232-Übertragung nicht sauber und durch die zusätzliche Zeit, die 
malloc verbrät, wird das irgendwie kompensiert.

Vielleicht hast Du auch an anderer Stelle ein Problem z.B. mit dem 
Stack.

Jetzt wird es wohl Zeit, den In-Circuit-Debugger auszupacken.

von blan (Gast)


Lesenswert?

also auch wenn ich malloc() schon am anfang der main-methode mache 
funktioniert es damit wunderbar. wie kann ich das denn debuggen lassen?

mfg blan

von Florian D. (code-wiz)


Lesenswert?

Je nachdem, welche Hardware Du verwendest, kannst Du ggf. mit einem 
JTAG-Debugger etwas machen.

Richtig, welche Hardware ist das überhaupt?

von blan (Gast)


Lesenswert?

ich hab ein ATmega8 und programmier ihn mit dem STK500.

mfg blan

von Florian D. (code-wiz)


Lesenswert?

Da kann ich leider nicht mehr weiterhelfen. Ich habe hier keinen ATmega8 
herumliegen und habe ehrlich gesagt, mit den 8-bit AVRs auch noch nicht 
viel gemacht. Nach meinem Kenntnisstand bietet aber das AVRSTUDIO eine 
Simulationsfunktion. Inwiefern die zu gebrauchen ist, weiß ich nicht. 
Ansonsten vielleicht das JTAGICE mkII nehmen und am STK500 anschließen. 
Ob das funktioniert und ob das den ATmega8 unterstützt, ist mir nicht 
bekannt.

Vielleicht hat ja jemand anderes die entspr. Hardware auch und kann das 
ggf. nachstellen.

von blan (Gast)


Lesenswert?

also scheinbar habe ich den fehler gefunden. er war in beiden 
fifo-buffern der gleiche. die funktion strlen() war der übeltäter - sie 
erwartet ein null-terminated string. wenn ich ein zeichen übergebe ist 
das logischerweise nicht null-terminated und so zähler er halt über das 
zeichen hinaus bis er auf ein nullbyte stoßt. so sieht jetzt meine 
push-funktion aus:
1
ISR( USART_RXC_vect )
2
{
3
  int8_t data[2];
4
  data[0] = uart_getc_nowait();
5
  data[1] = NULLBYTE;
6
7
  fifo_push( fifo_rx, data );
8
}

wenn jemand noch ein verbesserungsvorschlag hat - her damit :)

mfg blan

von Florian D. (code-wiz)


Lesenswert?

Man könnte fifo_push drei Parameter geben: den fifo, einen Pointer und 
eine Länge.

Dann noch schnell ein paar Makros gebaut
#define fifo_push_byte(x, y) fifo_push( x, &y, 1 )
#define fifo_push_string(x, y) fifo_push(x, y, strlen((char*)y))

Ein Wunder übrigens, dass das dann mit dem malloc geklappt hat.

von blan (Gast)


Lesenswert?

also wenn man schon etwas mehr code drin hat treten sehr komische fehler 
auf - hab ich testhalber mal probiert. ich denk da werden 
rücksprungadressen oder ähnliches überschrieben. also ich denke jetzt 
funktioniert es - vielen dank für die gute hilfe!

mfg blan

von Florian D. (code-wiz)


Lesenswert?

Noch ein paar Anmerkungen:

- Ich würde fragment.size (ist übrigens merkwürdig definiert) weglassen 
und auch der pop-Funktion eine Größe übergeben.

-
1
  if ( size % fragment )
2
    return NULL;
ist eigentlich Quatsch, da Du zum Schreiben ja eine andere Größe als zu 
Lesen verwendest und so oder so abgefragt wird, ob zum Lesen genug Daten 
bereitstehen.

- Wenn der Puffer groß ist, wäre ein Ringpuffer angebrachter, da das 
memmove für jeden Lesevorgang (gerade, wenn Du byteweise liest) viel 
Zeit verschwendet.

- Die Bezeichnungen push und pop erinnern eher an ein Stack und der 
macht nicht FIFO, sondern LIFO. Das nur nebenbei.

von blan (Gast)


Lesenswert?

vielen dank für die anmerkungen, man lernt nie aus :)
ich werde sie wenn ich zeit habe umsetzen.

mfg blan

Bitte melde dich an um einen Beitrag zu schreiben. Anmeldung ist kostenlos und dauert nur eine Minute.
Bestehender Account
Schon ein Account bei Google/GoogleMail? Keine Anmeldung erforderlich!
Mit Google-Account einloggen
Noch kein Account? Hier anmelden.