Probleme mit malloc()

[Icke M. (Firma: my-solution) (hendi)]

Hallo, ich hab mal wieder ein Problem. Wie einige hier schon mitbekommen 
haben hab ich mit C erst angefangen, deswegen fehlt mir hier und da noch 
der Durchblick. Ich möchte Daten, die ich von einem Browser bekomme über 
einen Webserver einlesen. Der TCP/IP stack ist der aus EasyWeb von 
Andreas Dannenberg, falls das jemandem was sagt. Er ist bei den 
Beispielen der Keil uvision dabei. Es funktioniert auch alles soweit 
ganz gut, ich kann Daten senden und empfangen. Da der Puffer zum 
Einlesen der Daten vom Ethernetcontroller begrenzt ist, erhalte ich bei 
längeren Daten fragmentierte Pakete. Der Webserver liest diese zyklisch 
aus. Ich will nun eigentlich nur die Daten, die die TCP/IP Schicht zur 
Verfügung stellt, aneinander hängen, da ich sie später parsen oder 
anderweitig verwenden will. Genau hier liegt auch mein Problem, wie 
gesagt auslesen klappt, aneinander hängen allerdings nicht.
Mein Code sieht so aus:

if(HTTPRxBuffer!=NULL&&sizeof(HTTPRxBuffer>0)){            //does HTTPRxBuffer exist? Is there some data in it? -->first cycle is through
    sendString("cycle größer 1");
     buf0 = (unsigned char *)malloc(sizeof(HTTPRxBuffer));                /*get memory to save old buffer*/
      if(buf0!=NULL){
      memcpy(buf0, HTTPRxBuffer, sizeof(HTTPRxBuffer));            //save content of old HTTPRxBuffer
      sendString("http nach buf0 schieben");
      sendString("Größe von buf0");
      sprintf(num1, "%d",sizeof(buf0));
      sendString(num1);
      }
    free(HTTPRxBuffer);                        //discard old HTTPRxBuffer
    HTTPRxBuffer = NULL;
    HTTPRxBuffer = (unsigned char *)malloc(sizeof(buf0)+TCPRxDataCount);        //get memory for new HTTPRxBuffer
      if(HTTPRxBuffer!=NULL&&buf0!=NULL){
      sendString("http buffer neu zusammensetzen");
      sprintf(num,"%d",sizeof(buf0));
      sendString(num);
      memcpy(HTTPRxBuffer, buf0, sizeof(buf0));              //copy old content to new HTTPRxBuffer
      memcpy(HTTPRxBuffer+sizeof(buf0), RxTCPBuffer, TCPRxDataCount);  //save new incomig traffic to HTTPRxBuffer
      free(buf0);                            //discard buf0
      buf0 = NULL;
    if(TCPRxDataCount<MAX_TCP_RX_DATA_SIZE)
      HTTPReadComplete = true;
      }
    if(HTTPRxBuffer==NULL)
    sendString("httprxbuffer ist NULL!");
    HTTPRxBufferSize = sizeof(HTTPRxBuffer);
    sendString("httprxbuffersize");
    sprintf(num,"%d",HTTPRxBufferSize);
    }
   else{
     sendString("1. cycle");
    HTTPRxBuffer = (unsigned char *)malloc(TCPRxDataCount);                   //first cycle, fetch memory for HTTPRxBuffer
    if(HTTPRxBuffer != NULL)
      memcpy(HTTPRxBuffer, RxTCPBuffer, TCPRxDataCount);         //put available data in the buffer
      if(TCPRxDataCount<MAX_TCP_RX_DATA_SIZE)
      HTTPReadComplete = true;
     HTTPRxBufferSize = sizeof(HTTPRxBuffer);  
   }

Meine Frage wäre nun, ob ich malloc falsch benutze? Kann man kurz 
hintereinander den Speicher von einem Pointer nehmen und dann gleich 
neuen zuweisen?
Zur Erklärung: HTTPRxBuffer-Puffer den ich füllen möchte, buf0-zur 
Zwischenspeicherung, TCPRxDataCount-Daten, die eingelesen werden 
könen(TCP/IP), sendString() sendet Daten an die serielle Schnittstelle, 
mit der ich die Daten überwache. HTTPRxBuffer lasse ich mir später im 
Programm damit ausgeben, aber es steht immer nur das drin, was im 
aktuellen RxTCPBuffer steht, den ich jeweils auslese.
So, nun hoffe ich, das sich das trotz der Länge jemand durchliest, danke 
schon mal dafür.
Bis dann

[Icke M. (Firma: my-solution) (hendi)]

Hab ich ganz vergessen, angelegt wird scheinbar immer was, also in die 
if!=NULL Bedingung spribgts immer rein.

[Karl H. (kbuchegg)]

> Meine Frage wäre nun, ob ich malloc falsch benutze? Kann man kurz
> hintereinander den Speicher von einem Pointer nehmen und dann gleich
> neuen zuweisen?

Ja, kann man.
Aber dein Code enthält viele Fehler und durch die Art und Weise
der Formatierung ist er nicht besonders leicht zu lesen. Mach
doch ein paar Leerzeichen rein um die Dinge optisch etwas
zu gruppieren. Sowas

      if(HTTPRxBuffer!=NULL&&buf0!=NULL){

muss doch nicht wirklich sein. Da muss ich erst mal eine Viertelstunde
parsen bis ich raus habe, dass du haben willst

      if( HTTPRxBuffer != NULL &&
          buf0 != NULL ) {

Zu den Fehlern.

Das hier

if(HTTPRxBuffer!=NULL&&sizeof(HTTPRxBuffer>0)){

macht mit Sicherheit nicht das was du denkst. Mach mal ein paar
Leerzeichen rein und sieh nach wie denn der > wirkt und in welchem
Zusammenhang er zum sizeof steht :-) (Mit etwas mehr Leerzeichen
wäre das wahrscheinlich nicht passiert)


Du erwartest zu viel von sizeof. sizeof kannst du
nur bei Dingen verwenden die zur Compilezeit schon ihre Größe haben.

zb

      memcpy(HTTPRxBuffer, buf0, sizeof(buf0));              //copy old content to new HTTPRxBuffer

das kopiert nicht den ganzen Bufferinhalt um, sondern nur
sizeof(buf[0]) Bytes. buf[0] ist vom Datentyp her ein unsigned
char, hat also die Länge 1. Daher wird auch nur 1 byte umkopiert.

Selbiges hier

    HTTPRxBuffer = (unsigned char *)malloc(sizeof(buf0)+TCPRxDataCount);        //get memory for new HTTPRxBuffer

buf0 ist ein Pointer. Abhängig davon wieviele Bytes ein Pointer
auf deinem System belegt ist das eine Zahl zwischen 2 und 4 aber
sicher nicht die Buffergröße die du hier

buf0 = (unsigned char *)malloc(sizeof(HTTPRxBuffer));                /*get memory to save old buffer*/

allokiert hast.

Wenn ich dir einen Rat geben darf: Fang mal damit an dir eine
Append Funktion zu schreiben, die die Aufgabe hat an einen
bestehenden Buffer neue Daten anzuhängen und bei Bedarf auch
neuen Speicher allokiert. Die Funktion sollte so verwendet
werden:
     Buffer = Append( Buffer, &BufferLen, Data, DataLen );

mit der Bedeutung: An Buffer (momentane Größe der Daten im Buffer
steht in BufferLen) die neuen Daten aus Data (deren Größe ist in
DataLen angegeben) anhängen. Der Returnwert der Funktion ist ein
Zeiger auf einen Speicherbereich in dem diese Aneinanderfügung
stattgefunden hat und BufferLen wurde in der Größe korrigiert
(daher auch die Übergabe als Zeiger). Append gibt auch einen
eventuell von Buffer bereits allokierten Speicher frei (Buffer
könnte ja auch NULL sein), lässt allerdings die Speicherallokierung
von Data in Ruhe.

Die Funktion hat also die Signatur

unsigned char* Append( unsigned char* OldData,
                       int* OldDataSize,
                       unsigned char* AppendData,
                       int AppendDataSize )
{
  ... Your code goes here
}

[Icke M. (Firma: my-solution) (hendi)]

Hallo Karl Heinz, wow, so schnell hätte ich nich mit einer Antwort 
gerechnet, aber umso besser :-). Ich werd mich morgen früh mal ran 
setzen deine Ratschläge zu befolgen. Das mit der Formatierung tut mir 
leid, ich bin mir da selbst noch nich ganz so einig, aber wie das 
Beispiel mal wieder zeigt muss ich da noch nachsteuern.
Ich dachte, das die Größe bei malloc() die Größe der Daten anlegt, die 
man speichern will, also z.b. 256B angelegt und nen Zeiger auf die erste 
Adresse zurückbekommen. So war mein Plan, naja, werd mir mal die 
Appendfunktion vornehmen, das is ne gute Idee.
So, nu aber gute Nacht

[... (Gast)]

Hi,

das mit malloc() hast Du durchaus richtig verstanden.
Nur das 'sizeof(...)', liefert nicht das, was Du erwartest.

CU

[Jörg W. (dl8dtl) (Moderator)]

Karl heinz Buchegger wrote:

> das kopiert nicht den ganzen Bufferinhalt um, sondern nur
> sizeof(buf[0]) Bytes. buf[0] ist vom Datentyp her ein unsigned char,
> hat also die Länge 1. Daher wird auch nur 1 byte umkopiert.

Naja, da stand nicht buf[0] sondern buf0.  Ändert aber nur minimal
was: buf0 ist (wie du einen Absatz tiefer festgestellt hast) ein
Pointer, damit ist sizeof(buf0) gleich 2, es werden also zwei Bytes
kopiert.

[Karl H. (kbuchegg)]

@Jörg

Da hatte ich wohl Tomaten auf den Augen :-) (War schon spät)

@Icke

Wie ... schon sagte, den malloc hast du schon richtig verstanden.
Was du aber noch intus kriegen musst: Wieviele Bytes du in einem
malloc allokoiert hast, musst du dir selbstin einer Variablen
merken. Es gibt keine C-Funktion mit der du rauskriegen könntest
wieviele Bytes sich hinter einem Pointer den du von malloc() bekommen
hast verbergen.

Die oben vorgeschlagene Funktion soll dir helfen diese Abstraktion
zu machen und zu orgaisieren. Denn der Funktion selbst ist es
völlig egal, ob sich in diesem Speicherbereich nun TCP/IP Daten
oder Telefonbücher tummeln. Alles was sie will ist: Pointer
auf die Daten + Länge der Daten. Diese beiden Informationen gehören
zusammen und sind untrennbar miteinander verknüpft (weswegen es
sich auch anbieten würde dafür einen eigenen Datentyp, eine struct,
zu erfinden).

[Icke M. (Firma: my-solution) (hendi)]

Also ich hab das jetzt mal ausprobiert, das append() hab ich implizit in 
der Funktion gemacht, wo ich auch vorher schon rumkopiert hab. 
Angehangen wird jetzt, ich hatte die Fkt. sizeof() einfach völlig falsch 
verstanden, ich merk mir jetzt wie du gesagt hast die Größe des Puffers 
beim letzten Durchlauf. Das klappt soweit, er bleibt bis jetzt aber noch 
beim dritten Durchlauf hängen, aber das liegt vermutich an irgendnem 
Fehler, den ich selbst heraus finde, wenn nicht werd ich euch noch mal 
fragen :-). Ansonsten danke für die schnelle Hilfe!!!

[Icke M. (Firma: my-solution) (hendi)]

So Jungs, das war echt ne schwere Geburt, aber jetzt klapts so wie ich 
mir das vorgestellt hab. Nach langem hin und her und ewiger Googelei kam 
die Erleuchtung. malloc() arbeitet auf dem Heap, gut zu wissen, der war 
bei mir nämlich gleich mal 0 bytes groß :( sehr dumme Sache. Google 
verwies mich dabei auch noch genau auf diese Seite... Damit war klar, 
dass er sich irgendwann mal aufhängt, da es vermutlich zum Overflow kam, 
oder sehe ich das falsch? Was ich nicht ganz verstehe ist, dass ich so 
um die 500Bytes in einem Rutsch locker lesen konnte, mit Rumkopiererei 
allerdings nicht. Hängt das mit der Fragmentierung zusammen? Aber warum 
hat malloc() dann nicht NULL zurückgegeben, oder war das nur Glück? 
Woher hat malloc() den Speicher genommen, als der Heap noch 0B war? Oder 
gibts da vielleicht noch irgendwo nen default Wert? (LPC2378)
Wie auch immer hab ihn jetzt auf 256B eingestellt, damit funktionierts 
erst mal ganz gut. Wenns dazu kritische Betrachtungen gibt immer her 
damit. Wenn ich den Speicher regelmäßig, nach meiner Bearbeitung lösche 
sollte es doch keine Fragmentierungsprobleme in Zukunft geben, oder?
So hier noch der Code, vielleicht bringts ja jemand anderem mal was, 
oder euch fällt noch was auf. Hab auch versucht Augenkrebs zu vermeiden.

if(TCPRxDataCount > 0){                //data to work with?

  if(HTTPRxBuffer != NULL){            //does HTTPRxBuffer exist?  -->first cycle is through

     buf0 = (unsigned char *) malloc(oldBufferSize);        //get memory to save old buffer
                    
      if(buf0 != NULL){
      memcpy(buf0, HTTPRxBuffer, oldBufferSize);            //save content of old HTTPRxBuffer      
      }

    free(HTTPRxBuffer);                        //discard old HTTPRxBuffer
    HTTPRxBuffer = NULL;
    HTTPRxBuffer = (unsigned char *) malloc(oldBufferSize + TCPRxDataCount);         //get memory for new HTTPRxBuffer

      if(HTTPRxBuffer != NULL&&
       buf0 != NULL){

      memcpy(HTTPRxBuffer, buf0, oldBufferSize);                //copy old content to new HTTPRxBuffer
      memcpy( (HTTPRxBuffer+oldBufferSize), RxTCPBuffer, TCPRxDataCount);    //save new incomig traffic to HTTPRxBuffer
      free(buf0);                                //discard buf0
      buf0 = NULL; 

    }
    if(HTTPRxBuffer == NULL)            //debugging
      sendString("httprxbuffer ist NULL!"); 

    oldBufferSize += TCPRxDataCount ;       //size of the new HTTPRxBuffer

    if(TCPRxDataCount < MAX_TCP_RX_DATA_SIZE){
      HTTPReadComplete = true;
      sendString("komplett ausgelesen");
      HTTPRxBufferSize = oldBufferSize;
      }      
    }
   else{
       HTTPRxBuffer = (unsigned char *) malloc(TCPRxDataCount);                   //first cycle, fetch memory for HTTPRxBuffer

    if(HTTPRxBuffer != NULL){
      memcpy(HTTPRxBuffer, RxTCPBuffer, TCPRxDataCount ); //put available data in the buffer
      sendString("httprxbuffer ist nicht null");  
    }

      oldBufferSize = TCPRxDataCount;
    cycle++;  
    if(TCPRxDataCount < MAX_TCP_RX_DATA_SIZE){
      HTTPReadComplete = true;  
      HTTPRxBufferSize = oldBufferSize;        //tell the user how many bytes are available       
      sendString("Komplett gelesen");
    }
   }
}
else{
   if(cycle > 0){
       HTTPReadComplete = 1;            //scenario, if last bufferread read exactly all left bytes out of the RxTCPBuffer
       HTTPRxBufferSize = oldBufferSize;
       }
}
      
if(HTTPRxBuffer != NULL){
  sendString("Daten auslesen");
  sendString((char *) HTTPRxBuffer);

if(HTTPReadComplete)
  clearHTTPRxBuffer();
}
        
     
}
void clearHTTPRxBuffer(){      //has to be called to get new HTTP data

free(HTTPRxBuffer);
HTTPRxBuffer = NULL;
HTTPReadComplete = false;
HTTPRxBufferSize = 0;
oldBufferSize = 0;
cycle = 0;
}

Wie gesagt, wenns was zu nörgeln gibt, meine Ohren sind offen, ansonsten 
sag ich mal bis zum nächsten Problem ;-) und nochmals danke!!!

[Patrick D. (oldbug)]

buf0 = (unsigned char *) malloc(oldBufferSize);

Der Cast ist unnötig und macht Dir möglicherweise mal das Leben unnötig 
schwerer ;-)

[Icke M. (Firma: my-solution) (hendi)]

Ich hatte mal irgendwo gelesen, dass es den Code lesbarer und besser 
nachvollziehbar macht, die Meinung teile ich, man weiß halt immer was 
drin ist und der Compiler meckert und man braucht nicht ewig suchen wo 
der Fehler ist, wenn was verhauen ist. Da ich wie gesagt am Anfang bin 
behalte ich das erst mal bei, bis mein Überblick besser ist.

[Karl H. (kbuchegg)]

Icke Muster wrote:
> Ich hatte mal irgendwo gelesen, dass es den Code lesbarer und besser
> nachvollziehbar macht,

was ist daran lesbarer bzw. besser nachvollziehbar?

Wenndu duokumentieren willst, dass hier unsigned char allokiert
werden, dann schreib das so:

buf0 = malloc(oldBufferSize * sizeof( unsigned char ) );

aber lass den cast weg.

> die Meinung teile ich, man weiß halt immer was
> drin ist

das weist du auch so. An dieser Stelle interessiert das
(zb. bei der Allokierung einer struct) aber auch niemanden.
Du willst an dieser Stelle genug Speicher für zb. eine struct Test
allokieren. Also schreibt man

  struct Test* pPtr;

  pPtr = malloc( sizeof( struct Test ) );

und hat damit dokumentiert, dass man Speicher für eine struct
allokiern will. Der cast bringt an dieser Stelle gar nichts,
weder an Sicherheit noch an dokumentarischem Wert.

Aber eigentlich ist mir an dieser Stelle auch relativ egal, dass
pPtr ausgerechnet auf eine struct Test zeigen soll. Das ist ein
Detail das ich an dieser Stelle gar nicht haben will. Eigentlich
will ich ja ausdrücken: Allokiere genug Speicher, so dass pPtr
entsprechend seinem Datentyp auf genau ein derartiges Objekt
zeigt, egal was das jetzt sein mag. In C schreibt sich das dann

  struct Test* pPtr;

  pPtr = malloc( sizeof( *pPtr ) );

das hat dann auch den Vorteil, dass sich diese Anweisung ganz
von alleine an veränderte Umstände anpasst, zb. wenn pPtr plötzlich
nicht mehr auf eine struct Test sondern auf eine struct FileHeader
zeigen soll:

  struct FileHeader* pPtr;

  pPtr = malloc( sizeof( *pPtr ) );

Siehst du? Keine Änderung beim malloc notwendig! Damit habe ich
hier eine mögliche Fehlerquelle ausgeschlossen. Und jeder Fehler
der per Design vermieden werden kann ist ein guter Fehler.

> und der Compiler meckert und man braucht nicht ewig suchen wo
> der Fehler ist, wenn was verhauen ist.

Ganz im Gegenteil.
Mit einem cast stellst du das Typ Prüfsystem des Compilers
ab und übernimmst selbst die 100% Verantwortung dafür, dass
alles richtig ist.

Das Problem:
Wenn du den Header nicht inkludierst, in dem malloc() definiert
ist, dann gelten Standardannahmen für den Funktionsaufruf. Zb.
die Annahme, dass die Funktion einen int zurückliefert (was sie
nicht tut. Sie liefert einen void*).
Wenn jetzt auf deinem System (zugegeben, auf einem AVR ist das nicht
so) ein int eine andere Größe hat als ein void *, dann hast du dir
mit dem vergessenen Header File einen Fehler eingebaut, der mit
ziemlicher Sicherheit in Folge zu einem kräftigen Programmabsturz
führen wird und der extrem schwer zu finden ist.

Dumm ist nur, dass der Cast den Compiler ruhigstellt -> Keine
Fehlermeldung

Lässt du aber den Cast weg, dann teilt dir der Compiler in diesem
Fall brav mit, dass er einen int nicht an einen Pointer zuweisen
kann und du merkst zumindest beim Compilieren, dass da was nicht
stimmt.

Cast sind Waffen! Sie sollten nur eingesetzt werden, wenn es
nicht anders geht. Hier in diesem speziellen Fall ist der Cast
völlig unnötig und in speziellen Fällen sogar kontraproduktiv,
weil er einen Fehler verstecken kann, der böse ins Auge gehen
kann.

[Uhu U. (uhu)]

Vor allem erschweren Casts die Wiederverwendung von Code ungemein - man 
muß ihn Zeile für Zeile nachvollziehen und überprüfen, ob die neue 
Maschine, auf der er laufen soll, dafür paßt.

Bei einigermaßen komplizierten Algorithmen kommt das u.U. einer 
Neuimplementierung gleich.

Zudem sind Fehler, die durch falsches casting entstehen, ziemlich gemein 
und oft sehr schwer zu finden.