cppCheck

• Hrochokobry
• memory leaks

cppCheck

• Knedlíci
• Lots of warnings like: Member variable '...' is not initialized in the constructor.
• memory leaks
• Possible leak in public function. The pointer 'mAesKey' is not deallocated before it is allocated. Found in ClientApplication/CryptoManager.cpp:147

cppCheck

• Sapiens
• [Client/DHGenerator.cpp:56]: (error) Instance of 'DHGenerator' object is destroyed immediately.
• memory leaks
• [Client/main.cpp:97]: (error) fflush() called on input stream 'stdin' results in undefined behaviour.

rats

• file rats_output.txt

Knedlíci

• Portability
• Qt
• Security
• Crypto manager
• Threading
• C++11
• English
• Other

Portability

• Linux: Windows.h: No such file or directory
• Windows: won't compile either
• project works only on Windows where int is of size 4

int connectionID;
...
if(sizeof(connectionID) == 4) {
    data[1] = connectionID & 0x000000ff;
    data[2] = (connectionID & 0x0000ff00) >> 8;
    data[3] = (connectionID & 0x00ff0000) >> 16;
    data[4] = (connectionID & 0xff000000) >> 24;
} else return 1;

Portability

#pragma once

• non-standard
• Qt Creator creates include guards automatically

Qt

• use Qt types consistently: QString, QByteArray...
• QByteArray reverse(const QByteArray& array) {...}
  • implicit sharing
  • copy-on-write
• own event loop:

while(true) {
    Sleep(10);
    qApp->processEvents();
}

• why not just "return a.exec();"?

Qt

• blocking calls:

return mSocket->waitForBytesWritten(10000);

Security

• poor random generation

srand(time(NULL));
//generovani klice
for(int i = sizeof(connectionID); i < dataSize; 
    i++) 
{
    data[i] = rand() % 256;
}

• sha256 hash - no integrity, instead use HMAC

Crypto manager

• int cpyStringToUnsignedCharArray(std::string str, unsigned char * array) {...}
  • use string.c_str() or string.data()
• std::chrono::milliseconds sleepTime(50); - in worker thread

Threading

• waiting for something, don't know for what...
• possible race condition

while(*end - *start > 0 && 
        *end - *start <= length)
        std::this_thread::sleep_for(sleepTime);

C++11

unsigned char *data = NULL;
bool mLoggedToServer = nullptr;

• what?
• also not set C++11 in server config - warning!

English

• czech language in the comments
• receive, not recieve
• prepare, not prepair

Other issues

• server mytcpclient.cpp - error

mytcpclient.cpp:58: error: taking address of 
    temporary [-fpermissive]
memcpy(mLastReicevedData, 
    &data[ID_LENGHT + RANDOM_BYTES_LENGTH + 4], 
    dataSize);                                                              

• solution:

memcpy(mLastReicevedData, data.data() + 
    (ID_LENGHT + RANDOM_BYTES_LENGTH + 4), 
    dataSize);

Hrochokobry

• Portability
• Security
• Qt
• C++
• Threading

Portability

• polarssl and polarSSL directories - same folder on Windows

Security

• poor random generation

key[i] = rand() % 256;

• using std::string for storing key

std::string buffer((const char*)key);

• std::string usually not used for general data, use QByteArray

Qt

• not using Qt types, for example QString and QByteArray
• inheriting from QThread
  • you don't need to do this
  • if you want to use QThread, then use obj.moveToThread(QThread);

C++

#define STR_IP( ip ) ip.toString().toStdString()

• please don't use macros in C++
• not needed if you would use qDebug() and QString
• comparing std::string as C-string
  • !strncmp( menu.c_str(), "1", 1)
  • menu.at(0) == '1'
• unnecessary copy of memory

void Client::SendMessage(std::string line) {..}

C++

• methods with uppercase first letter
  • void ClientThread::SendUserList()
  • We are not in C#!
• using malloc() in C++ - WHY?

Threading

• AES precomputation not in thread

Sapiens

• compiler g++, source files *.cpp
• C code with classes and std::cout
• indentation and formatting generally very bad
• long C code + bad formatting == unreadable

Be consistent

        ++ibuf;
        len--;

Avoid ugly shortcuts

    c2 = *ibuf++;

• looks like code from OpenSSL
• you don't want to write OpenSSL-like code

Format your code

• if you don't need to write space, it doesn't mean you shouldn't do it

    cout<<clientInfo.id<<"\t: "<<str<<endl;

• not so easy to quickly decode
• client.cpp
  • line 308
  • line 629
  • line 633 - replace blocks in switch for functions

Avoid unnecessary memory copies

    int  UserIf::message(char* msg, int len)
    {
        char* str = (char*)malloc(len+1);
                strncpy(str, msg, len);
                str[len] = 0;
                cout<<str<<endl;
        free(str);
         return SUCCESS;

    }

Learn how to use pointers

int Client::startSSL( unsigned char* response, 
    int * len)
...
response  = (unsigned char*)malloc(bytes);
    - memory allocated
memcpy(response, buf, bytes); 
    - something copied to the memory
...
return... - response never used and 
            memory is leaked

C++ is rich language

#define FALSE 0
#define TRUE !FALSE

• there are booleans in C++...

Careful with binary operators

session->e.crypt(data, (len+15)&~0xF, SEND);

• it is good practise to use a lot of parentheses in conjunction with binary operators
• they have low priority

Know your tools

• client.cpp:797

unsigned char *data; - some general data
strncpy(str, (char*)data, len);

• strncpy does not work with general data
• use memcopy or memmove
• data again unnecessary copied, printed and released...

Never implement your own crypto

    // setting n to a large prime number
    mpz_set_str (n,"9627533057541626274",10);

• a lot of work
• never secure
• Never implement your own crypto!
• Never implement your own crypto!
• Never implement your own crypto!

Follow convetions

• encryptor is a class, name of class should start with capital letter -> Encryptor

Keep things simple

• 200 unformatted lines in main() is not simple
• server.cpp - enormous switch should be divided into functions

Dynamically allocated memory is important

long clientIP[MAX_CLIENT_LIST];

• there are dynamically allocated containers in C++, for example std::vector

Thank you!