AuthDB

Tuxist1

Moin,

schreibe gerade an einer Datenbank für Authentifizierung und Session Handling.

Bin jetzt soweit die erste Techdemo präsentieren zu können.

Wollte wissen ob ich da auf dem richtigen Weg bin und Anregungen sammeln.

Habe auch schon ein Docker Container zum testen der Oberfläche:

docker run -p 9090:9090 tuxist/authdb

Die Administration der Datenbank läuft unterm Browser ab und ist unter Port 9090 erreichbar.

An der Client Api arbeite ich gerade und benutze hier für JSON. Sollte nächste Woche rundimänter funktionieren.

Wenn ihr fragen habt könnte ihr gerne fragen

Quiche Lorraine

Ich habe mal einen kurzen Blick in deinen Code geworfen.

Warum nutzt du manuelle Speicherverwaltung?

BTW: Lass doch mal bitte CppCheck drüberlaufen.

Tuxist1

danke werde ich nochmal machen mit Valgrind konnte ich noch keine anomalen Dinge festellen.

Tuxist1

@Quiche-Lorraine automatische Speicherverwaltung ist unter Musl schwerig weil die Stacksize dort 128kb nur groß ist und ich immer wieder mit abstürzen zu kämpfen hatte zum Beispiel mit auto_ptr weil der Stack immer wieder übergelaufen ist.

DNKpp

@Tuxist1 auto_ptr sollte man auch wirklich nicht mehr verwenden. Allgemein sieht dein Code irgendwie alt aus. Hab's mir jetzt nicht im Detail angesehen aber sowas hier

 std::vector<char> recv;

std::copy(curreq->RecvData.begin(),curreq->RecvData.end(),std::back_inserter(recv));

ist umständlich und sogar ineffizienter als (sofern curreq->RecvData ein konstant ermittelbare Größe hat).

 std::vector<char> recv{curreq->RecvData.begin(),curreq->RecvData.end()};

Das hier macht mich äußerst nervös und ich frage mich, was das mit der Stackgröße zu tun haben soll.

authdb::AuthBackend::AuthBackend(int type, const char* options,const char *domain) : _Type(type),_Options(options),_Domain(domain) {
    if(_Type==AuthBackendType::File){
        _Api = new class File(_Options,VERSION,_Domain);
    }else{
        throw AuthBackendError("unkown file backend");
    }
}

authdb::AuthBackend::AuthBackend(const AuthBackend &backend){
    if(backend._Type==AuthBackendType::File){
        _Api = new class File(backend._Options,VERSION,backend._Domain);
    }
}

authdb::AuthBackend::~AuthBackend(){
    if(_Type==AuthBackendType::File){
        delete ((class File*)_Api);
    }
}

Wildes rohes Zeigermanagement und herumgecaste ist eigentlich aus dem vorletzten Jahrzehnt.

Ohne das jetzt böse zu meinen aber kann es sein, dass du eigentlich aus C kommst?

Quiche Lorraine

@Tuxist1
Und std::unique_ptr bzw. std::shared_ptr funktionieren für dich nicht?

Quiche Lorraine

@Tuxist1 sagte in AuthDB:

weil die Stacksize dort 128kb nur groß ist

Also der folgende Ausdruck sizeof(std::make_unique<char>()); liefert bei mir 4 zurück. Also ist das eher kein Kandidat welcher Stack frisst.

Und 128 kByte ist schon ok, sofern man keine Rekursion macht.

Tuxist1

ich sollte ich mich mehr mit den neuen Sprach Standarts beschäftigen habt ihr recht.

C nutze ich auch gerne das stimmt. Wo mit überhaupt nicht zurecht komme ist Java ist mir zu abstrakt zu Umständlich.

Ich denke das werde ich alles nochmal überarbeiten sobald ich die API am laufen habe.

Teile meiner Biobiotheken habe auch schon 10 Jahre oder mehr auf den Buckel.

Das ich verkette Listen benutze liegt daran das ich es nicht mag Allokatoren zu schreiben finde das total umständlich und mit liste schneller und einfacher vielleicht habe ich da auch unrecht.

wob

Du kannst auch mal clang-tidy verwenden. Wenn du cmake aufrufst, verwende
cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ...deine Argumente...
Und dann kannst du clang-tidy datei.cpp -p build-directory ... checken.

Es kommt z.B. raus:

authdb/src/session.cpp:165:26: warning: Potential leak of memory pointed to by 'grpdat.gid' [clang-analyzer-cplusplus.NewDeleteLeaks]
  165 |         _lastData->_next=new SessionData(sid,userid,domainid,grpdat.gid,grpdat.count);
      |                          ^
authdb/src/session.cpp:105:11: note: Assuming 'rd' is >= 'end'
  105 |     while(rd<end){
      |           ^~~~~~
authdb/src/session.cpp:105:5: note: Loop condition is false. Execution continues on line 134
  105 |     while(rd<end){
      |     ^
authdb/src/session.cpp:134:18: note: Memory is allocated
  134 |     grpdat.gid = new uuid_t[grpdat.count];
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~
authdb/src/session.cpp:138:11: note: 'rd' is >= 'end'
  138 |     while(rd<end){
      |           ^~
authdb/src/session.cpp:138:5: note: Loop condition is false. Execution continues on line 158
  138 |     while(rd<end){
      |     ^
authdb/src/session.cpp:161:8: note: Assuming field '_firstData' is non-null
  161 |     if(!_firstData){
      |        ^~~~~~~~~~~
authdb/src/session.cpp:161:5: note: Taking false branch
  161 |     if(!_firstData){
      |     ^
authdb/src/session.cpp:165:26: note: Potential leak of memory pointed to by 'grpdat.gid'
  165 |         _lastData->_next=new SessionData(sid,userid,domainid,grpdat.gid,grpdat.count);
      |                          ^
authdb/src/session.cpp:255:5: warning: Argument to 'delete[]' is uninitialized [clang-analyzer-core.CallAndMessage]
  255 |     delete[] grpdat.gid;
      |     ^~~~~~~~~~~~~~~~~~~
authdb/src/session.cpp:215:5: note: Loop condition is true.  Entering loop body
  215 |     for(SessionData *cur=_firstData; cur; cur=cur->_next){
      |     ^
authdb/src/session.cpp:216:12: note: Assuming the condition is true
  216 |         if(uuid_compare(cur->_sid,sessionid)==0){
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
authdb/src/session.cpp:216:9: note: Taking true branch
  216 |         if(uuid_compare(cur->_sid,sessionid)==0){
      |         ^
authdb/src/session.cpp:218:13: note:  Execution continues on line 222
  218 |             break;
      |             ^
authdb/src/session.cpp:222:9: note: 'cursess' is non-null
  222 |     if(!cursess)
      |         ^~~~~~~
authdb/src/session.cpp:222:5: note: Taking false branch
  222 |     if(!cursess)
      |     ^
authdb/src/session.cpp:226:11: note: Assuming 'rd' is >= 'end'
  226 |     while(rd<end){
      |           ^~~~~~
authdb/src/session.cpp:226:5: note: Loop condition is false. Execution continues on line 255
  226 |     while(rd<end){
      |     ^
authdb/src/session.cpp:255:5: note: Argument to 'delete[]' is uninitialized
  255 |     delete[] grpdat.gid;
      |     ^~~~~~~~~~~~~~~~~~~

Du kannst mit -check=\* auch andere Checks aktivieren (mit dem * sind es aber zu viele, da dann auch viele Style-Dinge dabei sind, die man oft nicht will).
Da fällt dann sowas auf:

authdb/src/session.cpp:144:22: warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
  144 |         backend.read((unsigned char*)&cur,sizeof(AuthData::Record));

oder sowas:

authdb/src/session.cpp:232:22: warning: C-style casts are discouraged; use reinterpret_cast [google-readability-casting]
  232 |         backend.read((unsigned char*)&cur,sizeof(AuthData::Record));
      |                      ^~~~~~~~~~~~~~~~    
      |                      reinterpret_cast<unsigned char*>( )

Quiche Lorraine

BTW:
So wie es aussieht frisst die Funktion authdb::sha512::transform aus dem Stand 1 kByte Stack (lokale Parameter (w, wv,...) + Klassenvariablen (m_block,...).

BTW: Kryptographische Funktionen sollte man nicht selbst programmieren, da zu trickreich und fehleranfällig. Hast du wenigstens eine Testreihe für sha512.cppaufgebaut?

Tuxist1

@Quiche-Lorraine soweit bin ich noch nicht gekommen ist ja erst eine Techpreview soll mit in die unit tests fließen mit welchen ich nocht nicht angefangen habe aber danke für den Hinweiß

Tuxist1

        struct Groups{
            size_t  count = 0;
            uuid_t *gid = nullptr;
        } _member;

gegen std::array tauschen ?

wob

@Tuxist1 sagte in AuthDB:

ist ja erst eine Techpreview soll mit in die unit tests fließen mit welchen ich nocht nicht angefangen habe

Sorry, aber so wird das nie funktionieren und dir auch nicht helfen. Schreib den Test gleich zusammen mit der Funktion! Später macht man es eh nicht. Bei sowas wie dem sha-Code könnte man die Tests auch problemlos vor der Funktion schreiben. (Manche sagen, dass man das immer machen soll, aber ich schreibe Tests auch mal gerne nach der Funktion, besonders wenn ich um die problematischen Stellen der Implementierung bescheid weiß und diese ganz gezielt testen will). Was noch niche funktioniert hat, ist die Tests erst viel später zu schreiben. Du willst doch auch von einer gut getesteten Codebase profitieren, z.B. bei einem Refactoring sicher(er) sein, dass noch alles geht.

Quiche Lorraine

@Tuxist1 sagte in AuthDB:

gegen std::array tauschen ?

Würde ich so empfehlen. Vor allen Dingen kannst du so auf einmal dein Struct kopieren, ohne dass du Seiteneffekte bekommst.

Ich würde aber auch zuerst empfehlen Testreihen zu schreiben, um dein Verhalten festzuzurren. Wie ein Korsett schnürst du dann dein Programm ein und siehst bei Änderungen direkt, ob sich dein Verhalten verändert hat.

Tuxist1

die session.cpp habe ich jetzt stark modifiziert und an den sha code gehe morgen.

wob

Wenn ich mir die 17 Commit alle mit dem Titel "fixed session.cpp" angucke, frage ich mich, ob das wirkliche Fixes sind oder teilweise eher style-Improvements.

z.B. im letzten Commit:

-            std::shared_ptr<unsigned char[]>tmp(new unsigned char[cur.datasize]);
+            const std::shared_ptr<unsigned char[]>tmp(new unsigned char[cur.datasize]);
             backend.read(tmp.get(),cur.datasize);
             username=reinterpret_cast<char*>(tmp.get());
         }

Statt const vs non-const stellen sich mir hier ganz andere Fragen:

Warum überhaupt ein shared pointer?! Der wird nicht gesharet! Sollte unique_ptr sein!
Shared- und unique-Pointer werden mit std::make_shared und std::make_unique angelegt, nicht manuell mit new! Siehe R22 und R23 in https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-make_shared
Ist überhaupt sichergestellt, dass backend.read etwas nullterminiertes zurückgibt?
Könnte man nicht dem Backend direkt beibringen, in einen string zu lesen, sodass man die tmp gar nicht braucht?