Jo, das ist übel.
Nach delete this; gehört alsbald ein return; . Zumindest - wie ja schon aus der Parashift FAQ zitiert wurde - darf man danach keine Member mehr angreifen.
Und natürlich stellt sich die Frage - wie ja auch schon geschrieben wurde - ob denn das restliche Programm "mitbekommt" dass das sslSocketS Objekt jetzt futsch ist.
----
Aber weiter (Anmerkungen als Code-Kommentar)...
void sslSocketS::recvSocket ( char *data ) {
if ( !validSocket() ) {
info ( "Socket ist broken!" );
delete this;
// Hier fehlt wie gesagt ein return;
};
// data ist ein Parameter - wieso tun wir das Array dann löschen und neu anlegen?
// Und wie soll der Rest des Codes mitbekommen dass der data Zeiger geändert wurde?
if ( data != NULL )
delete[] data; // Immer neue Zeile nach if/for/while/... und schön Einrücken bringt Karma-Punkte
data = new char[ MAXRECV + 1 ];
memset ( data, 0, MAXRECV + 1 );
ssize_t size = ssl_read ( m_ssl, (unsigned char*) data, MAXRECV + 1 ); // Siehe unten
if ( size == POLARSSL_ERR_SSL_PEER_CLOSE_NOTIFY ) {
delete[] data; // Hm. Hier also "delete data;" - wieso dann oben beim "if ( !validSocket() )" kein "delete data;" ?
data == NULL; // hier ist ein "=" zuviel. Davon abgesehen dass das Nullsetzen eines Parameters auch keiner mitbekommen wird
info ( "Client disconnected" );
delete this;
// Hier folgt zwar nix mehr, aber ein return; wäre trotzdem nett
}
else if ( size == POLARSSL_ERR_NET_CONN_RESET ) {
delete[] data;
data = NULL;
warning ( "Reset by Peer!" );
delete this;
// Hier folgt zwar nix mehr, aber ein return; wäre trotzdem nett
}
// Nu' haben wir schön size auf Fehler gecheckt - aber wie bekommt der Aufrufer mit wie viel empfangen wurde?
// Wenn wir [c]MAXRECV[/c] statt [c]MAXRECV + 1[/c] an [c]ssl_read[/c] übergeben würden,
// dann könnte man wenigstens noch davon ausgehen dass es immer ein Nullterminierter String ist (der könnte dann höchstens zu kurz ausfallen, falls der Client mal ein Nullbyte schickt, aber zumindest wäre kein Bufferoverrun möglich).
// Aber so... übel.
}
OK, weiter...
Die Sache mit "delete this;" und einer globalen Verwaltungs-Instanz ist ja grundsätzlich noch OK, WENN die Klasse sich im Destruktor brav bei der Verwaltungs-Instanz "abmeldet".
Trotzdem bleibt die Frage, ob es OK ist wenn recvSocket einfach delete this; macht. Kommt die Aufrufende Stelle damit klar? Die kann das schliesslich nur mitbekommen, wenn sie nach dem Aufruf von recvSocket bei der Verwaltungs-Instanz nachfragt ob das Objekt noch registriert ist. Dazu muss entweder ein Zeiger auf das Objekt übergeben werden (was UB ist: Zeiger auf nicht-mehr-existierende Objekte sind ganz einfach ungültige Zeiger, und ungültige Zeiger übergeben/vergleichen/... ist eben UB). Oder aber der Socket muss anhand einer ID oder ähnlichem identifiziert werden. Auf jeden Fall wäre es langsam.
Und wie gesagt die Geschichte mit dem data Parameter... ich sehe da keine Möglichkeit wie das jemals funktionieren sollte.
Vorschlag:
char const* GetPolarSSLErrorMessage(int errorCode) {
switch (errorCode) {
case POLARSSL_ERR_SSL_PEER_CLOSE_NOTIFY:
return "Client disconnected";
case POLARSSL_ERR_NET_CONN_RESET:
return "Reset by Peer!";
default:
return "Unknown error";
}
}
void sslSocketS::breakSocket(char const* message) {
info(message);
m_broken = true;
}
bool sslSocketS::isBroken() {
if (!m_broken && !validSocket())
breakSocket("Socket ist broken!");
return m_broken;
}
size_t sslSocketS::recvSocket(char* data, size_t size) {
if (isBroken() || size == 0)
return 0;
assert(data != 0);
int const rc = ssl_read(m_ssl, reinterpret_cast<unsigned char*>(data), size);
if (rc < 0) {
breakSocket(GetPolarSSLErrorMessage(rc));
return 0;
}
else
return static_cast<size_t>(rc);
}
Das Löschen und De-Registrieren verlagerst du dann an die Stelle die recvSocket aufruft. Wenn du sämtliche sslSocketS Funktionen mit einem if (isBroken()) nix tun und passenden Wert zurückgeben ausstattest musst du auch nicht nach jedem Aufruf einer sslSocketS Memberfunktion prüfen ob der Socket jetzt im Eimer ist, sondern u.U. sogar nur an einer einzigen Stelle.
Wenn du meinst dass es anders schöner/besser/toller wäre, dann zeig mal etwas mehr Code her. z.B. eben wo und wie recvSocket nun wirklich aufgerufen wird.
EDIT: OK, gerade erst gesehen... die checkSocket Funktion. Der Puffer ist dort NULL, weil du den Zeiger "by value" übergibst -- so kann die Aufrufende Funktion natürlich nicht mitbekommen was da passiert ist.
Aber wieso überhaupt in recvSocket jedes mal einen Puffer anlegen? Ist doch Verschwendung, leg den 1x an (als Member von ClientS ) und verwende ihn wieder.
Und... das sieht so aus als ob du ClientS von sslSocketS abgeleitet hättest. Das ist zwar eine Idee auf die Anfänger oft kommen, aber kein gutes Design. Ein Client (bzw. eine ClientConnection) "ist" kein Socket, sie "hat" einen Socket. -> sslSocketS sollte ein Member von ClientS sein, und keine Basisklasse.
EDIT2: Die Lösung für deine ganzen Probleme hier ist aber recht einfach: Lern C++ (bzw. Programmieren im Allgemeinen)
