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_tdata;
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?
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.
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.
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
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).
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
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:
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?
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
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.
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
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
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
return1;
3
else
4
return2;
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.
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
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.
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
>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).
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.
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
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
> (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.
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.
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.
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_tdata[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
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.
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
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
returnNULL;
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.