IP Range Port Scanner

I've been working on this project to learn networking and concurrency as well as C++11 practices. I'm just looking for a general code review.

```
#define _WINSOCK_DEPRECATED_NO_WARNINGS

#include
#include
#include
#include
#include
#include

#pragma comment(lib, "ws2_32.lib")

using namespace std;

typedef enum
{
NONE,
T_SCAN
} Thread_Type;

typedef struct
{
HANDLE tHandle;
Thread_Type tType;
SOCKET tSock;
} Single_Thread;

typedef struct
{
SOCKET Sock;
string IPAddress;
short int port;
short int threadNum;
short int totalThreads;
string *ptrIP; // Pointer To The IPAdress Above
} Scan_Job;

typedef struct
{
short int port;
bool isOpen;
function Port_Function;
} Port_Struct;

vector quenceThreads; // I Use this to push threads
Single_Thread hThreads[256];

// Generic Banner Grabber
// Works On Any Non SSL Protocol That Begins With A Send()
// Tested On: FTP
void Generic_Recv_Banner_Grabber(Scan_Job sJob)
{
SOCKET Sock;
char recvBuf[256];

// Define Socket & Make Sure Its Valid
Sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
if (Sock == INVALID_SOCKET)
{
// error handling code
}

// Setup Connection Struct
sockaddr_in sockAddr;
sockAddr.sin_family = AF_INET;
sockAddr.sin_port = htons(sJob.port);
sockAddr.sin_addr.S_un.S_addr = inet_addr(sJob.IPAddress.c_str());

// Connect to the server
if (connect(Sock, (sockaddr*)(&sockAddr), sizeof(sockAddr)) != 0)
{
cout c_str());

// Convert ULONG To Network Byte Order
addr1 = ntohl(addr1);

// Incremement By 1
addr1 += 1;

// Convert Network Byte Order Back To ULONG
addr1 = htonl(addr1);

// Convert ULONG Back To in_addr Struct
paddr.S_un.S_addr = addr1;

// Convert Back To String
*host = inet_ntoa(paddr);
}

// This Function Checks If A Remote Port Is Open
bool PortCheck(SOCKET sock, string ip, int port)
{
struct sockaddr_in sin;

There is a reason these warnings are there:

#define _WINSOCK_DEPRECATED_NO_WARNINGS

Whatever function is causing this error should be replaced with a modern equivalent.

Don't do this.

using namespace std;

There is a big description of why here: Why is “using namespace std;” considered bad practice?.

From this:

typedef struct
{
    HANDLE      tHandle;
    Thread_Type tType;
    SOCKET      tSock;
} Single_Thread;

I can guess you are using windows threads. Why not use the C++ threads? They are cross platform, and more importantly, they work like C++ threads should (the Windows threading library is C based and thus horrible to use).

Why would you want to retain the address of a member that you have locally?

string *ptrIP; // Pointer To The IPAdress Above

This is "C" code.

typedef enum
{
    NONE,
    T_SCAN
} Thread_Type;

Type names, structure names and enumeration names are all in the same namespace in C++. So there is no need to use a typedef here.

enum Thread_Type
{
    NONE,
    T_SCAN
};

struct Single_Thread
{
    HANDLE      tHandle;
    Thread_Type tType;
    SOCKET      tSock;
};

Now you are using both threading systems:

// Windows threads
tHandle = CreateThread(nullptr, 0, function, (LPVOID)param, 0, &id);

// C++ threads
vector quenceThreads;   // I Use this to push threads
quenceThreads.push_back(thread(hPorts[i].Port_Function, hExpScanJob));

Pick one (and pick the C++ version).

Not all services listening on a port are going to reply.

cout << "Successfully Connected To " << sJob.IPAddress << ":" << sJob.port << " To Do Banner Grab!" << endl;
    if (recv(Sock, recvBuf, sizeof(recvBuf), 0) != SOCKET_ERROR)
    {
        cout << "Data: " << recvBuf << endl;
    }

This is just as likely to hang for ever as give you data. I believe that even HTTP connections are going to wait for you to make the first move. You have to send a message before they will reply (I could be wrong on that).

This:

// Return Result
if (i <= 0) {
    return(false);
}
else {
    return(true);
}

is an overly verbose way of saying

return i > 0;

The number of threads is usually best set to a number close to the number of cores on the machine.

hScanJob.totalThreads   = 30;

Unless you have some heavy metal here this number is way to big. You should create a couple of threads. Then reuse the threads. So create a job for each port then get your threads to pick up jobs from the job queue.

Admittedly creating a large number of jobs will protect you against time you errors. But thats because you are using threading and sockets incorrectly together. You should open all the sockets in one thread then use select to see who is listening then dispatch readers when you get a connection.

Search for the C10K problem

StackExchange Code Review Q#102556, answer score: 3