C++ Winsock Networking Tutorials [04-24-2017]

[Pindrought]

I thought we were closing the socket, thus why i just used server disconnect case to call the Original Client::Connect() function. I see the error and i should have added the close connection and attempt to open a new one in my case. I see the error statement that i should have also included in the client connect with the new bool. This was the issue i was having a hard time learning, thank you so much. I look forward to future tutorials. I was also thinking of adding a database system to the client, but have not fully decided because there are several different library that could be used, OLEdb, ADO, ODBC, trying to figure out which one is the easiest to learn( for others). Also security is another issue, depending on what a db has stored, depends on different laws and different way to protect that information. I'm familiar with American laws, Other country laws may be different and i'm not up to par on that. What are your thoughts about it?

I'm not familiar with the laws regarding databases. I never looked into it.

However, one thing to note is that if you do use a database, only the server should ever connect to that database. The client will send the server information about queries it wants to perform (not exact select statements, but you know packets with the contents of what you're searching) and the server will validate that the query should be allowed, perform the query, and return whichever results back to the client. From what I understand, if the client is able to connect to the DB, then it becomes vulnerable to hackers executing any query that they want on your db.

Example:

Client goes to connect, sends server a packet that contains Account Name / Account Password.
Server queries the account table with the user's account name and compares the account password result.
If the result matches, allow the user in.
If the result does not match, send a disconnection packet that will also give an invalid pw message on the client side to the user, and also manually close the connection from the server's side in case the user is using a hacked client that would ignore the disconnection packet.

[Ferbguy]

Well the database system uses what is called a stored procedure and u just send the values to the database call the procedure. Using any type of select statements or queries inside code opens it to SQL injection attacks. Also, you never send plain text passwords. You hash it and compare the string.

[Pindrought]

Well the database system uses what is called a stored procedure and u just send the values to the database call the procedure. Using any type of select statements or queries inside code opens it to SQL injection attacks. Also, you never send plain text passwords. You hash it and compare the string.

Exactly. Completely agree.

[Pindrought]

Uploaded tutorial 11 solution to use vectors instead of a static array if anyone wants to take a look at it. Video will be coming sometime this week just been really busy.

[Pindrought]

Uploaded tutorial 11 video, reuploaded tutorial 11 solution as it had a couple of bugs

[Pindrought]

Uploaded tutorial 12 video/solution

[reductor]

Just watching your tutorials, good work, figured I'd provide some feedback/suggestions.

Tutorial 1:

• Should avoid mixing GUI and console, instead of using MessageBox use std::cerr
• Instead of using exit(1) within main just return 1
• The comment on inet_addr( "127.0.0.1") says broadcast locally, this has nothing to do with broadcasting
• The length of the socket address (SOCKADDR_IN) is needed because SOCKADDR_IN can be of a different size for different address families, and this can be copied and used without knowledge of the exact address length from the family
• IPv6 isn't really a different socket, just a different address
• Do not use NULL for protocol within bind() use 0 if you want this to be automatic, otherwise be explicit and use IPPROTO_TCP -- NULL/nullptr should only be used for pointers
• You should avoid re-using the address used for bind() within accept(), this can confuse people that they are related.
• If your not going to use the address from accept() just leave it and the length empty (Also solves my previous item)
• For errors you should prefer std::cerr over std::cout, otherwise if someone wants to redirect output from your application they will end up with errors and standard output mixed
• For connect() you can just use sizeof(addr) to avoid another local variable
• Do not use NULL for flags on send() or recv() use 0 -- NULL is for pointers
• For send() you say "The second pointer is a char pointer to the address", a pointer is an address this is just a "char pointer" -- What you said is more "char **"
• Your leaking the thread handle, you should use CloseHandle()

Tutorial 2

• Threads per client are very inefficent, and threading is easy to get wrong
• You should use std::thread instead of CreateThread
• Your function pointer being a different signature is going to lead to undefined behaviour, your just lucky the ABI is allowing this, but will break at some point. The client is going to have some issues if you return from it because of the left over parameters.
• Again dwStackSize and dwCreationFlags in CreateThread should be 0 and not NULL
• Your usage of threads is unsafe and undefined behaviour, the ConnectionCounter and Connection array can result in re-ordered reads/writes and potential torn reads/writes
• While send/recv should be atomic from different threads I would avoid this

Tutorial 3

• Instead of using sizeof(int) when your just wanting the sizeof a variable use sizeof(variableName) -- e.g. sizeof(buffer.length())
• int can vary in size on different platforms, so should avoid using it in any stream (file or network), additionally endian ness should be taken care of -- Might be able to get away with this for hobby code, but worth nothing that it's not good
• send and recv isn't alway going to send and recv the full amount specified, you should always check the return value -- This is especially important with TCP
• You can recieve straight into a std::string by resizing the string and using string::data() as the pointer to store the value in

Tutorial 4

• Instead of using prefixed enum just use 'enum class'
• The reason for having to put {} around your case statement when it defines variables is because that variable would otherwise be visible to the following case statements, and if it was to jump into one of those then what should the value be?
• sizeof(Packet) can vary on different compilers and platforms, you should use the new C++11 feature of specifying the size of an enum
• To share the enum you should have probably created one solution with two applications, and then store the enum within a library

Tutorial 5

• You should not prefix variables with an underscore, this is reserved for the compiler
• For recv() you need to also handle 0 for graceful disconnect
• GetString should probably not prefix \0 and use the char* assignment, instead just use assign() with the size, otherwise resize the string and use it within recv()
• Should return false within ProcessPacket() when the packet was unknown

Tutorial 6

• For the thread and using a member function, thats what the LPVOID is for on CreateThread
• More unsafe threading
• Broadcast is a completely different term in networking, bind to all would be the term you should use
• That static global means a file local variable, you should never have a static variable in a header file

Tutorial 7

• For sendall you should use 'const char *' instead of 'char *' and casting it when using std::string::c_str()
• Big-Endian isn't necesarily "proper" for application level protocol, many applications use little-endian because most of their clients and servers are little endian

Tutorial 8

• Endian isn't specifically about the process storing integers but the process reading/writing them from memory
• The non-endian processors (or non-twos compliment) are older machines (e.g. signed-magnitude)

Tutorial 10 (Pre-tutorial #1)

• Why not just std::list

Tutorial 10 (Pre-tutorial #2)

• Thread is from C++11 (Released in 2011, known and implemented well before that)
• The reference was not an issue just what it ended up with due to the race condition
• Should use std::lock_guard instead of lock and unlock

Tutorial 10

• Instead of doing size() > 0 use empty
• Instead of new and delete everywhere, use unique_ptr

Tutorial 11

• Re-using over destroying and re-creating can make things hard to maintain and is error prone
• Instead of creating a new connection and using push_back use emplace_back
• Should use make_shared, and for connection unique_ptr is probably more appropriate
• You still have race conditions with your mutexes, e.g. Server::PacketSenderThread can be using connections when it is changing (and various other places)

[albinopapa]

Why should 0/NULL not be interchangeable? Since the introduction of nullptr, NULL for a null'd pointer has lost it's meaning in my eyes, and NULL is just a macro defined to be 0, so I think it's perfectly fine to use NULL and 0 interchangeably.

The usage of std::thread over the Win API CreateThread seems odd to me, just because I'm ignorant of implementation specifics. Some of the C++ implementation in MSVC are just wrappers for the Windows API, so if programming for Windows it would make since to just use the Win API right? I understand about if you want portability, you should use the C++ standard library though.

Just out of curiosity reductor, what do you do for a living? You have a lot of suggestions about coding style and are pretty knowledgeable of C++ in general, are you in the industry?

[Pindrought]

Thank you very much for the feedback/suggestions reductor. I don't think I know it all or anything so I really appreciate all of the information I can get. The only reason I was using NULL when I was using it, was because all of the code I had learned from was using NULL. Now that I see that it may be confusing since it does not return a ptr, I will refrain from using NULL in the future.

I have some questions about your notes.

Tutorial 1:

• The comment on inet_addr( "127.0.0.1") says broadcast locally, this has nothing to do with broadcasting What is the correct terminology for what I was saying? I meant that only clients on the same router would be able to connect since the server was only "broadcasting" (I know now that this is the wrong term) locally.

Tutorial 2

• Your function pointer being a different signature is going to lead to undefined behaviour, your just lucky the ABI is allowing this, but will break at some point. The client is going to have some issues if you return from it because of the left over parameters. Is there any way you could clarify this for me? I'm going to look into it, but i'm not sure what you mean about the function ptr being a different signature.

Tutorial 6

• That static global means a file local variable, you should never have a static variable in a header file I know it is rigged up, but how do you suggest accessing the Server class from the threads? This was the simplest work around I could find, but if there is a right way to do it, i'd rather take that route. Should I just use std::thread and pass a ptr to the server class in the thread parms?

Tutorial 11

• You still have race conditions with your mutexes, e.g. Server::PacketSenderThread can be using connections when it is changing (and various other places) Could you clarify this part? I am having issues seeing how there is a race condition in the PacketSenderThread.

Thanks a lot reductor.

[reductor]

Why should 0/NULL not be interchangeable? Since the introduction of nullptr, NULL for a null'd pointer has lost it's meaning in my eyes, and NULL is just a macro defined to be 0, so I think it's perfectly fine to use NULL and 0 interchangeably.

NULL has not lost it's meaning of being used for pointers, in-fact it should not be used at all now and nullptr should only be used. Using NULL makes it unclear to the reader what the argument being specified is. Additionally you will potentially get warnings from different compilers and static analysis.

The usage of std::thread over the Win API CreateThread seems odd to me, just because I'm ignorant of implementation specifics. Some of the C++ implementation in MSVC are just wrappers for the Windows API, so if programming for Windows it would make since to just use the Win API right? I understand about if you want portability, you should use the C++ standard library though.

std::thread has many benefits:
* Using RAII doesn't get the handle leak currently in this code
* Usage of a callable prevents the undefined behaviour with the casting of function pointers
* Cross-platform
* Easier to use

The Windows API is old, and quiet awful if you have something from the standard exists to do what you want use it instead of using the Windows API, it will generally be better for alot of the same reasons. The Windows API is designed for usage with "C" so has all the issues of a C API. If you want to use Windows specific APIs maybe look at WinRT

Just out of curiosity reductor, what do you do for a living? You have a lot of suggestions about coding style and are pretty knowledgeable of C++ in general, are you in the industry?

I've been programming C++ for around 16 years (Released my first project back in 2000 -- Bots for Half-life 1), and also do it professionally working as a Senior Software Engineer on a large (Guiness world record breaking) MMO engine.

Thank you very much for the feedback/suggestions reductor. I don't think I know it all or anything so I really appreciate all of the information I can get. The only reason I was using NULL when I was using it, was because all of the code I had learned from was using NULL. Now that I see that it may be confusing since it does not return a ptr, I will refrain from using NULL in the future.

Not a problem, I'm always open to giving feedback. If you want me to review anything just send me a PM.

I have some questions about your notes.

Tutorial 1:

• The comment on inet_addr( "127.0.0.1") says broadcast locally, this has nothing to do with broadcasting

What is the correct terminology for what I was saying? I meant that only clients on the same router would be able to connect since the server was only "broadcasting" (I know now that this is the wrong term) locally.

This is called the loopback address ( INADDR_LOOPBACK ), this does not make it so only clients on the same router can see the connected clients it means only that local machine can access it via it's loopback interface.

Tutorial 2

• Your function pointer being a different signature is going to lead to undefined behaviour, your just lucky the ABI is allowing this, but will break at some point. The client is going to have some issues if you return from it because of the left over parameters.

Is there any way you could clarify this for me? I'm going to look into it, but i'm not sure what you mean about the function ptr being a different signature.

Code: Select all

CreateThread(NULL, NULL, (LPTHREAD_START_ROUTINE)ClientThread, NULL, NULL, NULL);

void Client::ClientThread()

The issue here is that the function is actually called with an LPVOID argument, causing it to be left on the stack (or within a register), this is undefined behaviour and can cause any number of things (Crashes, Issues when thread finishes, Unicorns to fly)

Tutorial 6

• That static global means a file local variable, you should never have a static variable in a header file

I know it is rigged up, but how do you suggest accessing the Server class from the threads? This was the simplest work around I could find, but if there is a right way to do it, i'd rather take that route. Should I just use std::thread and pass a ptr to the server class in the thread parms?

For global variables exposed to multiple different files you should use

Code: Select all

// server.h
extern Server * pServer;

// server.cpp
Server * pServer;

However for this you should be using std::thread and pass the pointer to the class via parameters, this is well defined, type-safe, and clear what is going on.

Tutorial 11

• You still have race conditions with your mutexes, e.g. Server::PacketSenderThread can be using connections when it is changing (and various other places)

Could you clarify this part? I am having issues seeing how there is a race condition in the PacketSenderThread.

Server::ListenForNewConnection can be adding a new item to the 'connections' vector which can move around the internal pointers within the vector. At the same time PacketSenderThread can be using the same pointer while iterating the vector. Not to mention all the other places where your using connections.

To simplify things and chances of race conditions you should have ClientHandlerThread take an argument for std::shared_ptr<Connection> (using std::thread), and it should stop using connection[id] as everytime this happens it should have the mutex. As you have a combination of read/write for connections you could use std::shared_mutex then just unique_lock for your writes/resizes and shared_ptr for any reads.