Memoryleak komme nicht weiter



  • Bei dem debuggen meiner libary bekomme ich einen für mich unverständlichen Memoryleak aber an der stelle passiert nichts aufregendes.

    kompletter source code https://github.com/Tuxist/libhttppp

    const char* HttpHeader::getHeader(){
      if(_Header)
        delete[] _Header;
      _HeaderSize=((_HeaderDataSize+_HeadlineLen)+((4*_Elements)+3));
      _Header=new char[(_HeaderSize+1)];
      std::copy(_Headline,_Headline+_HeadlineLen,_Header);
      size_t pos=_HeadlineLen;
      for(HeaderData *curdat=_firstHeaderData; curdat; curdat=curdat->_nextHeaderData){
        pos+=(snprintf(_Header+pos,(_HeaderSize-pos),
    		   "%s: %s\r\n",curdat->_Key,curdat->_Value));
      }
      snprintf(_Header+pos,_HeaderSize,"\r\n");
      _Header[_HeaderSize]='\0';
      std::cerr << _Header;
      return _Header;
    }
    

    GET /images/header.png HTTP/1.1
    Host: 127.0.0.1:8080
    User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
    Accept: image/png,image/;q=0.8,/;q=0.5
    Accept-Language: de,en-US;q=0.7,en;q=0.3
    Accept-Encoding: gzip, deflate
    Referer: http://127.0.0.1:8080/
    Connection: keep-alive
    Requesturl: /images/header.png
    ==781== Invalid write of size 1
    ==781== at 0x56C93F2: vsnprintf (vsnprintf.c:117)
    ==781== by 0x56A7E21: snprintf (snprintf.c:33)
    ==781== by 0x405FEC: libhttppp::HttpHeader::getHeader() (http.cpp:144)
    ==781== by 0x406560: libhttppp::HttpResponse::send(libhttppp::Connection
    , char const*, unsigned long) (http.cpp:223)
    ==781== by 0x405445: Controller::Controller(libhttppp::Connection*) (httpsysinfo.cpp:90)
    ==781== by 0x404AB9: libhttppp::Queue::RequestEvent(libhttppp::Connection*) (httpsysinfo.cpp:105)
    ==781== by 0x407587: libhttppp::Queue::Queue(libhttppp::ServerSocket*) (epoll.cpp:97)
    ==781== by 0x40704D: libhttppp::HttpD::runDaemon() (httpd.cpp:67)
    ==781== by 0x4055E9: HttpConD::HttpConD(int, char**) (httpsysinfo.cpp:117)
    ==781== by 0x404B87: main (httpsysinfo.cpp:126)
    ==781== Address 0x5a0e49a is 0 bytes after a block of size 90 alloc'd
    ==781== at 0x4C298A0: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==781== by 0x405F0C: libhttppp::HttpHeader::getHeader() (http.cpp:137)
    ==781== by 0x406560: libhttppp::HttpResponse::send(libhttppp::Connection*, char const*, unsigned long) (http.cpp:223)
    ==781== by 0x405445: Controller::Controller(libhttppp::Connection*) (httpsysinfo.cpp:90)
    ==781== by 0x404AB9: libhttppp::Queue::RequestEvent(libhttppp::Connection*) (httpsysinfo.cpp:105)
    ==781== by 0x407587: libhttppp::Queue::Queue(libhttppp::ServerSocket*) (epoll.cpp:97)
    ==781== by 0x40704D: libhttppp::HttpD::runDaemon() (httpd.cpp:67)
    ==781== by 0x4055E9: HttpConD::HttpConD(int, char**) (httpsysinfo.cpp:117)
    ==781== by 0x404B87: main (httpsysinfo.cpp:126)
    ==781==
    ==781== Invalid write of size 1
    ==781== at 0x56CD244: _IO_default_xsputn (genops.c:475)
    ==781== by 0x569D28C: vfprintf (vfprintf.c:1323)
    ==781== by 0x56C9408: vsnprintf (vsnprintf.c:119)
    ==781== by 0x56A7E21: snprintf (snprintf.c:33)
    ==781== by 0x405FEC: libhttppp::HttpHeader::getHeader() (http.cpp:144)
    ==781== by 0x406560: libhttppp::HttpResponse::send(libhttppp::Connection*, char const*, unsigned long) (http.cpp:223)
    ==781== by 0x405445: Controller::Controller(libhttppp::Connection*) (httpsysinfo.cpp:90)
    ==781== by 0x404AB9: libhttppp::Queue::RequestEvent(libhttppp::Connection*) (httpsysinfo.cpp:105)
    ==781== by 0x407587: libhttppp::Queue::Queue(libhttppp::ServerSocket*) (epoll.cpp:97)
    ==781== by 0x40704D: libhttppp::HttpD::runDaemon() (httpd.cpp:67)
    ==781== by 0x4055E9: HttpConD::HttpConD(int, char**) (httpsysinfo.cpp:117)
    ==781== by 0x404B87: main (httpsysinfo.cpp:126)
    ==781== Address 0x5a0e49a is 0 bytes after a block of size 90 alloc'd
    ==781== at 0x4C298A0: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==781== by 0x405F0C: libhttppp::HttpHeader::getHeader() (http.cpp:137)
    ==781== by 0x406560: libhttppp::HttpResponse::send(libhttppp::Connection*, char const*, unsigned long) (http.cpp:223)
    ==781== by 0x405445: Controller::Controller(libhttppp::Connection*) (httpsysinfo.cpp:90)
    ==781== by 0x404AB9: libhttppp::Queue::RequestEvent(libhttppp::Connection*) (httpsysinfo.cpp:105)
    ==781== by 0x407587: libhttppp::Queue::Queue(libhttppp::ServerSocket*) (epoll.cpp:97)
    ==781== by 0x40704D: libhttppp::HttpD::runDaemon() (httpd.cpp:67)
    ==781== by 0x4055E9: HttpConD::HttpConD(int, char**) (httpsysinfo.cpp:117)
    ==781== by 0x404B87: main (httpsysinfo.cpp:126)
    ==781==
    ==781== Invalid write of size 1
    ==781== at 0x56C941C: vsnprintf (vsnprintf.c:122)
    ==781== by 0x56A7E21: snprintf (snprintf.c:33)
    ==781== by 0x405FEC: libhttppp::HttpHeader::getHeader() (http.cpp:144)
    ==781== by 0x406560: libhttppp::HttpResponse::send(libhttppp::Connection*, char const*, unsigned long) (http.cpp:223)
    ==781== by 0x405445: Controller::Controller(libhttppp::Connection*) (httpsysinfo.cpp:90)
    ==781== by 0x404AB9: libhttppp::Queue::RequestEvent(libhttppp::Connection*) (httpsysinfo.cpp:105)
    ==781== by 0x407587: libhttppp::Queue::Queue(libhttppp::ServerSocket*) (epoll.cpp:97)
    ==781== by 0x40704D: libhttppp::HttpD::runDaemon() (httpd.cpp:67)
    ==781== by 0x4055E9: HttpConD::HttpConD(int, char**) (httpsysinfo.cpp:117)
    ==781== by 0x404B87: main (httpsysinfo.cpp:126)
    ==781== Address 0x5a0e49c is 2 bytes after a block of size 90 alloc'd
    ==781== at 0x4C298A0: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==781== by 0x405F0C: libhttppp::HttpHeader::getHeader() (http.cpp:137)
    ==781== by 0x406560: libhttppp::HttpResponse::send(libhttppp::Connection*, char const*, unsigned long) (http.cpp:223)
    ==781== by 0x405445: Controller::Controller(libhttppp::Connection*) (httpsysinfo.cpp:90)
    ==781== by 0x404AB9: libhttppp::Queue::RequestEvent(libhttppp::Connection*) (httpsysinfo.cpp:105)
    ==781== by 0x407587: libhttppp::Queue::Queue(libhttppp::ServerSocket*) (epoll.cpp:97)
    ==781== by 0x40704D: libhttppp::HttpD::runDaemon() (httpd.cpp:67)
    ==781== by 0x4055E9: HttpConD::HttpConD(int, char**) (httpsysinfo.cpp:117)
    ==781== by 0x404B87: main (httpsysinfo.cpp:126)
    ==781==
    HTTP/1.1 200 OK
    Content-Type: image/png
    Content-Length: 294834
    Connection: Keep-AliveHTTP Note: switch to send mode!
    HTTP Note: switch to recv mode!



  • Sieht recht grauenhauft aus. Fange erstmal mit Smartpointer an.



  • Wenn dir das Tool diesen Code-Abschnitt zeigt, gehe ich davon aus, dass irgendwo getHeader() aufgerufen wird, jedoch der zurückgegebene Header nicht mit delete[] freigegeben wird.

    Gib doch einfach einen std::string zurück, der verwaltet dann intern das Memory und du musst dich nicht explizit darum kümmern.

    BTW: delete und delete[] darf mit einem nullptr aufgerufen werden und bleibt ohne Effekt. D. h. die if -Abfrage vor dem delete[] ist überflüssig.



  • Die Berechnung von _HeaderSize (BTW: nicht erlaubter Name, da Unterstrich + Großbuchstabe) sieht mir zweifelhaft aus. Warum berechnest du das überhaupt von Hand und benutzt nicht std::string?



  • hier wirds es aufgerufen:

    void HttpResponse::send(Connection* curconnection,const char* data, size_t datalen){
      _HeadlineLen=snprintf(_Headline,512,"%s %s\r\n",_Version,_State);
      setData("Content-Length",datalen);
      const char *header=getHeader();
      curconnection->addSendQueue(header,getHeaderSize());
      if(datalen!=0)
        curconnection->addSendQueue(data,datalen);
    }
    

    mit unterstrich makiere ich zusätzlich private variablen



  • das funkioniert erstmal

    std::stringstream hstream;
      hstream << _Headline;
      size_t pos=_HeadlineLen;
      for(HeaderData *curdat=_firstHeaderData; curdat; curdat=curdat->_nextHeaderData){
        hstream << curdat->_Key << ": " << curdat->_Value << "\r\n";
      }
      hstream << "\r\n";
      std::string hdat=hstream.str();
      _Header=new char[hdat.length()];
      std::copy(hdat.c_str(),hdat.c_str()+hdat.length(),_Header);
      std::cerr << _Header;
      return _Header;
    


  • Wie bereits gesagt, std::string und unique_ptr sind die Lösungen.



  • 🙄

    Du gewinnst dadurch aber nichts.. das ist dir schon klar, oder? Ein delete[] fehlt und das fehlt nach deiner Änderung immer noch. Oder eben: std::string zurückgeben!



  • Tuxist schrieb:

    das funkioniert erstmal

    std::stringstream hstream;
      hstream << _Headline;
      size_t pos=_HeadlineLen;
      for(HeaderData *curdat=_firstHeaderData; curdat; curdat=curdat->_nextHeaderData){
        hstream << curdat->_Key << ": " << curdat->_Value << "\r\n";
      }
      hstream << "\r\n";
      std::string hdat=hstream.str();
      _Header=new char[hdat.length()];
      std::copy(hdat.c_str(),hdat.c_str()+hdat.length(),_Header);
      std::cerr << _Header;
      return _Header;
    

    sehe ich das richtig, dass es sich beim Header einfach um eine Ansammlung von chars handelt?
    Wenn ja: dann nimm einfach std::string _Header; und dort kopierst du dann das Ergebnis rein.
    Wenn du auch 0-Zeichen brauchst, dann verwende statt einem String ein Byte-Array, am einfachsten std::vector<char> _Header; in die Klassendefinition einfügen.

    Man kann natürlich auch mit new und delete fehlerfrei arbeiten, es ist aber viel schwieriger und man muss sehr aufpassen, auch immer delete aufzurufen. Daher am besten soweit möglich darauf verzichten. Und dein Anwendungsfall ist problemlos ohne new und delete machbar.



  • std::string kann problemlos nullbytes speichern.
    Das Problem an manuellem delete ist vor allem die fehlende Exceptionsicherheit.



  • Also weisst Du jetzt die (Maximal-)Länge des Strings vorher oder nicht?

    _HeaderSize=((_HeaderDataSize+_HeadlineLen)+((4*_Elements)+3));
    _Header=new char[(_HeaderSize+1)];
    

    Reicht das um da mehrfach

    "%s: %s\r\n",curdat->_Key,curdat->_Value
    

    rein zu drucken? Das erscheint mir schon fragwürdig.

    Definitiv hast Du aber in Z. 12

    snprintf(_Header+pos,_HeaderSize,"\r\n");
    

    nur noch Headersize-pos+1 Bytes Platz (inkl. '\0').

    Mit ein bisschen Sorgfalt sollte das eigentlich fliegen.



  • die informationen greife ich vorher ab:

    HttpHeader::HeaderData *HttpHeader::setData(const char* key, const char* value){
      for(HeaderData *curdat=_firstHeaderData; curdat; curdat=curdat->_nextHeaderData){
        if(strncmp(curdat->_Key,key,curdat->_Keylen)==0){
           return setData(key,value,curdat);
        }
      }
      if(!_firstHeaderData){
        _firstHeaderData=new HeaderData(key,value);
        _lastHeaderData=_firstHeaderData;
      }else{
        _lastHeaderData->_nextHeaderData=new HeaderData(key,value);
        _lastHeaderData=_lastHeaderData->_nextHeaderData;
      }
      _HeaderDataSize+=(_lastHeaderData->_Keylen+_lastHeaderData->_Valuelen);
      _Elements++;
      return _lastHeaderData;
    }
    

    oder verändere sie entsprechend

    void HttpHeader::deldata(const char* key){
      HeaderData *prevdat=NULL;
      for(HeaderData *curdat=_firstHeaderData; curdat; curdat=curdat->_nextHeaderData){
        if(strncmp(curdat->_Key,key,curdat->_Keylen)==0){
          if(prevdat){
            prevdat->_nextHeaderData=curdat->_nextHeaderData;
            if(_lastHeaderData==curdat)
              _lastHeaderData=prevdat;
          }else{
            _firstHeaderData=curdat->_nextHeaderData;
            if(_lastHeaderData==curdat)
              _lastHeaderData=_firstHeaderData;
          }
          _Elements--;
          _HeaderDataSize-=(curdat->_Keylen+curdat->_Valuelen);
          curdat->_nextHeaderData=NULL;
          delete curdat;
          return;
        }
        prevdat=curdat;
      }
    }
    
    HttpHeader::HeaderData *HttpHeader::setData(const char* key, const char* value,
    					    HttpHeader::HeaderData *pos){
      if(pos){
        _HeaderDataSize-=(pos->_Keylen+pos->_Valuelen);
        delete[] pos->_Key;
        delete[] pos->_Value;
        pos->_Keylen=strlen(key);
        pos->_Valuelen=strlen(value);
        pos->_Key=new char[pos->_Keylen+1];
        pos->_Value=new char[pos->_Valuelen+1];
        std::copy(key,key+(pos->_Keylen+1),pos->_Key);
        std::copy(value,value+(pos->_Valuelen+1),pos->_Value);
        _HeaderDataSize+=(pos->_Keylen+pos->_Valuelen);
        return pos;
      }else{
        setData(key,value);
      }
      return pos;
    }
    


  • Und was erwartet du jetzt?



  • die berrechung der größe vom char array



  • *hust*
    Warum benutzt du immer noch kein std::string?



  • weil ich alles später char blöcke zerteilen muss für sento und recvfrom

    ConnectionData *Connection::addSendQueue(const char*data,size_t datasize){
      size_t written=0;
      for(size_t cursize=datasize; cursize>0; cursize=datasize-written){
        if(cursize>BLOCKSIZE){
          cursize=BLOCKSIZE;
        }
        if(!_SendDataFirst){
          _SendDataFirst= new ConnectionData(data+written,cursize);
          _SendDataLast=_SendDataFirst;
        }else{
          _SendDataLast->_nextConnectionData=new ConnectionData(data+written,cursize);
          _SendDataLast=_SendDataLast->_nextConnectionData;
        }
        written+=cursize;
      }
      _SendDataSize+=written;
      return _SendDataLast;
    }
    
    ConnectionData *Connection::resizeSendQueue(size_t size){
      return _resizeQueue(&_SendDataFirst,&_SendDataLast,&_SendDataSize,size);
    }
    
    ConnectionData *Connection::_resizeQueue(ConnectionData** firstdata, ConnectionData** lastdata,
    					 size_t *qsize, size_t size){
      ConnectionData *firstdat=*firstdata;
      while(firstdata && size!=0){
        size_t delsize=0;
        if(size<=firstdat->getDataSize()){
           delsize=firstdat->getDataSize();
           ConnectionData *deldat=*firstdata;
           *firstdata=firstdat->_nextConnectionData;
           if(deldat==*lastdata)
             *lastdata=*firstdata;
           deldat->_nextConnectionData=NULL;
           delete deldat;
        }else{
           delsize=size;
           firstdat->_DataSize-=size;
        }
        size-=delsize;
        *qsize-=delsize;
      }
      return firstdat;
    }
    


  • Wie meinen? Von einem std::string kannst du dir mir data() oder c_str() einen zeiger auf die Daten holen. Du kannst komplett das gleiche machen wie dein new/delete-Gewurschtel, nur gibt es automatisches Aufräumen und Exceptionsicherheit gratis dazu. Wer manuell Speicher verwaltet, hat schon verloren und wird von den Javamännern ausgelacht. Siehst du ja gerade selbst.



  • gebe ich dir ja prinzipiel recht in dem fall soll es die bibliothek machen. Damit man es späteer nicht mehr selber machen muss. Recv und Send arbeiten mit festen Blockgrößen anders gar nicht realisierbar also brauche ich einen block basierten memorypool den ich schon gebaut habe. Bei aufbau habe ein bißchen mich an tinyxml2 orentiert.



  • Selbst wenn du einer Bibliothek char-Blöcke übergeben müsstest - und ich sehe nicht, warum das hier der Fall sein sollte, feste Blockgrößen sind schon mal kein Grund -, kannst du immer noch std::string zum Zusammensetzen der Daten nutzen und den fertigen String dann in char-Blöcke kopieren. Du könntest also fast in deinem gesamten Code auf dieses new-delete-Gefrickel mit manuell berechneten Arraygrößen verzichten. Versuchs doch wenigstens mal!



  • das hat jetzt funktioniert keine warum beim vorberrechnen die fehler gemacht hat.

    size_t HttpHeader::getHeaderSize(){
      size_t hsize=0;
      for(HeaderData *curdat=_firstHeaderData; curdat; curdat=curdat->_nextHeaderData){
        hsize+=curdat->_Keylen+curdat->_Valuelen;
      }
      return hsize;
    }
    

Anmelden zum Antworten