22
\$\begingroup\$

Intro

A couple of weeks ago I finished a Python implementation of a multithreaded port checker and I wasn't quite happy with the result I've got (speed). I needed it to be faster, so I've built another one using CPP (and a bit of C).

Here is the Python code, if anyone wants to see it.

At the moment, I didn't implemented the threading module for my script because I'd like to get some advice on what would be the wisest choice (while asking for an answer which adds this part is forbidden here, I wouldn't mind seeing an example of it)


Description

The program takes as command line arguments the following:

  • a text file which contains one domain name per line
  • the port number for which the above domains will be checked
  • number of threads
  • the timeout
  • a text file where the domains which have the port open will be written (along with their IP address) e.g: google.com:172.217.22.46

What the program is currently doing is to translate each domain into an IP address, and check if the given port is opened or not. If it's opened, write the domain:ip to an output file.

The program can be compiled and run using the below commands:

g++ port_checker.cpp -o checker
./checker domains.txt 80 2 2 output.txt

Code

#include <iostream>
#include <fstream>
#include <vector>
#include <sys/socket.h>
#include <netdb.h>
#include <arpa/inet.h>
#include <unistd.h>

std::vector<std::string> get_domains_from_file(std::string domains_file) {
    std::vector<std::string> domains_array;
    std::ifstream file(domains_file);
    std::string domain;

    if (!file.is_open()) {
        std::cerr << "Unable to open domains file!" << std::endl;
        std::exit(-1);
    }

    while (file >> domain) {
        domains_array.push_back(domain);
    }

    return domains_array;
}

int main(int argc, char *argv[]) {
    struct hostent *h;
    struct sockaddr_in servaddr;

    int sd, rval;

    if (argc != 6) {
        std::cerr << "The number of arguments should be 5 e.g: ./checker domains.txt [port] [threads] [timeout] output.txt" << std::endl;
        std::exit(-1);
    }

    std::string domains_file = argv[1];
    std::string output_file  = argv[5];
    int port                 = atoi(argv[2]);
    int threads              = atoi(argv[3]);
    int timeout              = atoi(argv[4]);

    std::vector<std::string> domains = get_domains_from_file(domains_file);

    std::ofstream myfile;
    myfile.open(output_file);

    for (int i = 0; i < domains.size(); i++) {
        if ((h=gethostbyname(domains[i].c_str())) == NULL) {
            std::cerr << "Error when using gethostbyname" << std::endl;
            std::exit(-1);
        }
        // std::cout << inet_ntoa(*((struct in_addr *)h->h_addr)) << std::endl;

        sd = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
        if (sd == -1) {
            std::cout << "Error when trying to create socket !" << std::endl;
            std::exit(-1);
        }

        memset(&servaddr, 0, sizeof(servaddr));
        servaddr.sin_family = AF_INET;
        servaddr.sin_port = htons(port);

        memcpy(&servaddr.sin_addr, h -> h_addr, h -> h_length);

        rval = connect(sd, (struct sockaddr *) &servaddr, sizeof(servaddr));

        if (rval == -1) {
            // std::cout << "Port is closed for domain: " << domains[i] << std::endl;
            continue;
        }

        else {
            myfile << domains[i] << ":" << inet_ntoa(*((struct in_addr *)h->h_addr))  << std::endl;
        }
        close(sd);
    }
    return 0;
}

What I'd like the most to get out of this review is:

  • how can I improve the performance ? (that's what concerns me first)
  • is there any other way of checking for an opened port ?
  • how can my code be improved in terms of readability / best practices ?

NOTE: Bear that this is my first C++ program in a very long time, so make sure you add as much information as you can in your review.

\$\endgroup\$
  • \$\begingroup\$ Hi! Usually people search by C++ tag, so since you've used up all five, could you replace the least significant one with C++ tag? C++11 would be a good candidate, you can just mention it at the top of the post. \$\endgroup\$ – Incomputable Feb 19 '17 at 8:31
  • 2
    \$\begingroup\$ This sort of thing is completely network-bound, python would be as good as C++ (using the same approach). And python would probably not leak open sockets like you're doing. \$\endgroup\$ – Mat Feb 19 '17 at 8:33
  • 1
    \$\begingroup\$ As you are checking multiple hosts, the best performance improvement will come from using asynchronous network APIs (e.g. with the help of Boost.Asio) \$\endgroup\$ – D. Jurcau Feb 19 '17 at 9:18
  • 1
    \$\begingroup\$ Since you have two solutions now because the first wasn't fast enough, did you benchmark both as well? \$\endgroup\$ – Mast Feb 19 '17 at 20:26
  • 1
    \$\begingroup\$ @Mast I'll do this these days for sure ! \$\endgroup\$ – яүυк Feb 19 '17 at 20:28
30
\$\begingroup\$

Checking TCP ports and multi-threading

Your performance should be fine for a sequential program. Another way to check whether the TCP port is open is by just sending the initial SYN and wait for the ACK. That way, you don't have to do the complete TCP handshake. However, that also means that you have to implement that part of the TCP protocol yourself. Also, some platforms don't allow using raw IP sockets for that for restricted users, which can pose another problem.

However, you cannot implement multi-threading at the moment, at least not over your current for loop. gethostbyname is not thread-safe. You have to use another function there, otherwise you will end up with data races and therefore undefined behaviour. GNU provides gethostbyaddr_r, which is an extension of the POSIX standard.

That being said, let's have a look at your current code.

Proper includes

#include <fstream>
#include <vector>
#include <sys/socket.h>
#include <netdb.h>
#include <arpa/inet.h>
#include <unistd.h>

You're missing several includes. cout and cerr need <iostream>, memcpy and memset need <cstring>, exit needs cstdlib.

Be more user friendly

We can use your get_domains_from_file as a case-study:

std::vector<std::string> get_domains_from_file(std::string domains_file) {
    std::vector<std::string> domains_array;
    std::ifstream file(domains_file);
    std::string domain;

    if (!file.is_open()) {
        std::cerr << "Unable to open domains file!" << std::endl;
        std::exit(-1);
    }

    while (file >> domain) {
        domains_array.push_back(domain);
    }

    return domains_array;
}

At the moment, you simply exit(-1) whenever you encounter a problem. I see two problems there:

  1. The exit code doesn't yield any information, apart from "it worked" or "it failed"
  2. You cannot recover the error, for example if you want to keep running even if you failed to create a socket for the 6000th domain.

Consider alternative input and output streams

Also, since we're currently in UNIX land, your program isn't very pipe-friendly. One can only get the domains from a file, but in a real-world scenario, it's likely that you have some logs which you want to check, e.g.

grep "acme" /var/log/attacks | ./domain-checker 80 4 2

We can at least make your program pipe-ready, if we use an istream here:

std::vector<std::string> get_domains_from_stream(std::istream & in) {
    std::vector<std::string> domains_array;
    std::string domain;

    while (in >> domain) {
        domains_array.push_back(domain);
    }

    return domains_array;
}

You can implement your other function with get_domains_from_stream:

std::vector<std::string> get_domains_from_file(std::string domains_file) {
    std::ifstream file(domains_file);

    if (!file.is_open()) {
        std::cerr << "Unable to open domains file!" << std::endl;
        return {};
    } else {
        return get_domains_from_stream(file);
    }
}

Note that I've returned an empty vector in this case. It is more or less a point of preference here, whether to return an empty resource, throw an exception, or do something else. But from a user's point of view, the "I've specified an empty file, whoops" and "I've specified a non-existing file, whoops" are the same kind of error: the user gave the wrong argument to your program.

Better usage message and input handling

While we're at error messages, you could improve your argument usage error message:

std::cerr << "The number of arguments should be 5 e.g: "
          << arg[0] << " domains.txt [port] [threads] [timeout] output.txt" 
          << std::endl;

You have no idea what the application is called on the user's PC, so just use arg[0] for that.

Speaking of arguments, how does your program work with invalid input?

$ ./a.out domains.txt abc def ghi blarg.out

No error message, no exception. That's due to atoi, which simply returns 0. Prefer std::stoi instead, which throws an exception if something is off:

struct hostent *h;             // hm, what are these?
struct sockaddr_in servaddr;   // where do you use them?

int sd, rval;                  // what are these ints?

const std::string domains_file = argv[1];
const std::string output_file  = argv[5];
const int port                 = std::stoi(argv[2]);
const int threads              = std::stoi(argv[3]);
const int timeout              = std::stoi(argv[4]);

By the way, my criticism about the input also holds for your output. cout is usually fine for output. If a user wants to write to a file, they can just use redirection:

$ grep "acme.evil" | ./checker 80 4 2 > evil.ip

Limit the scope of your variables as much as possible

Still in the same section of your program, you define int sd, int rval, struct hostent *h; and other's far too early. You always try to limit the scope of your variables. Therefore, get rid of them at that point. Also, use const a little bit more often to ensure that you don't change things. It will also help the compiler to optimize your program in several cases. Not necessarily in your current one, but it's usually a good idea.

The for-loop

Now, after above discussion, let's have a look at your for loop. Since you use C++11, consider a range-based for-loop:

for(const auto & domain : domains){
    struct hostent * h = gethostbyname(domain.c_str());

I don't think a std::exit is appropriate here. A small typo in the first line of your domain.txt shouldn't prevent the rest from getting checked. However, a little bit more information would be great:

    if (h == nullptr) {
        std::cerr << "Error when using gethostbyname: "
                  << hstrerror(h_errno)
                  << " (on domain: " << domain << ")"
                  << std::endl;
        continue;
    }

Note that hstrerror and h_errno are considered obsolete, but so is gesthostbyname.

A resource leak

You forgot to close sd if rval is -1. That can lead to a "too many handles" error, and you cannot continue to scan all those ports. One way to fix that would be not to use continue:

if(rval != -1){
    myfile << domains[i] << ":" << inet_ntoa(*reinterpret_cast<struct in_addr *>(h->h_addr))  << std::endl;
}
close(sd);

However, that's also error prone. Enter RAII:

class socket_wrapper {
public:
    explicit socket_wrapper(int fd) : m_fd(fd) {}
    ~socket_wrapper(){ 
        if(m_fd != -1) ::close(m_fd);
    }
    socket_wrapper() = delete;
    socket_wrapper(const socket_wrapper &) = delete;
    socket_wrapper & operator=(const socket_wrapper &) = delete;

    socket_wrapper(socket_wrapper && other) : m_fd(other.m_fd) {
        other.m_fd = -1;
    }
    socket_wrapper & operator=(socket_wrapper && other){
        this->close();
        m_fd = other.m_fd;
        other.m_fd = -1;
    }
    bool is_valid() const { return m_fd != -1; }

    void close() {
        ::close(m_fd);
        m_fd = -1;
    }
    int fd() const { return m_fd; }

private:
    int m_fd;
};

This is basically a wrapper around an int. However, the most important part is that it will automatically call close on that int, so you cannot forget it accidentally. This will make your code more robust. Note that the move operations are just a small bonus and for this an overkill. You probably won't need them. Still, it's important to forbid copying that wrapper. We end up with:

    const auto sd = static_cast<socket_wrapper>(socket(PF_INET, SOCK_STREAM, IPPROTO_TCP));
    if (sd.is_valid()) {
        std::cout << "Error when trying to create socket !" << std::endl;
        std::exit(-1);
    }

    memset(&servaddr, 0, sizeof(servaddr));
    servaddr.sin_family = AF_INET;
    servaddr.sin_port = htons(port);

    memcpy(&servaddr.sin_addr, h -> h_addr, h -> h_length);

    rval = connect(sd.fd(), reinterpret_cast<struct sockaddr *>(&servaddr), sizeof(servaddr));

    if (rval == -1) {
        // std::cout << "Port is closed for domain: " << domains[i] << std::endl;
        continue;
    }        
    myfile << domains[i] << ":" << inet_ntoa(*reinterpret_cast<struct in_addr *>(h->h_addr))  << std::endl;

We don't have to worry that we forgot to close our socket, since it will now get closed automatically. Note that the generated code will be pretty much the same as your current one. It's just safer.

Use the proper cast

You probably saw it already in the code above. Use the proper cast. A C-style cast should be the last resort.

A remark on performance

There are two bottlenecks in your program:

  1. Getting the address info for a domain name.
  2. Connecting to the given address.

Note that you can get address info on the other domains while you're connecting. So for a first implementation of multi-threading, you could get the address info in one thread, and connect in another. Connect them via a thread-safe, closable queue.

However, due to its blocking nature, the connect call may block for some time if the port isn't open. If you want to stay in POSIX land, you have to resort to multiple threads connecting at once. A worker pool might be suitable for this. Certain libraries like Boost.ASIO provide you asynchronous IO operations, which make this easier. Boost also provides thread-safe lockfree queues, so that you don't have to implement them yourself, if you're really racing for the last bit of performance.

But as always: before you start to optimize, profile your program. For a sequential program, there's not really anything else you can do at the moment. You read a single domain name, you get its network address, you try to connect, and you remember the result. And that's exactly what you did.

\$\endgroup\$
  • \$\begingroup\$ Shouldn't << arg[0] << "domains.txt [port] [threads] [timeout] output.txt" have a space before domains.txt? I'd think that otherwise it'd be missing the space in the output (eg ./checkerdomains.txt ...), but I'm not a C dev so there might be a trailing space that I'm not aware of. \$\endgroup\$ – Pokechu22 Feb 19 '17 at 17:39
  • \$\begingroup\$ @Pokechu22 no, you're correct. Fixed. \$\endgroup\$ – Zeta Feb 19 '17 at 17:39
  • 4
    \$\begingroup\$ This is a superb review that covers a lot of different aspects with well written examples. Kudos. \$\endgroup\$ – isanae Feb 19 '17 at 17:59
  • \$\begingroup\$ That's an incredible answer @Zeta. I didn't expect anything else from you. I'll apply the changes you suggested tomorrow. Thanks ! \$\endgroup\$ – яүυк Feb 19 '17 at 19:49
  • \$\begingroup\$ Nitpick: for the socket_wrapper you use snake case, so, why do you the use camel case for isValid? \$\endgroup\$ – someonewithpc Feb 20 '17 at 12:48
4
\$\begingroup\$

Here are some things that may help you improve your program.

Use the required #includes

The code uses memcpy and memset which means that it should #include <cstring>. It's important to have all of the appropriate #include lines in your program to assure portability even if it compiles without them on your particular machine. Also missing is <string>.

Don't mislead the user

The usage string for the program shows "[port] [threads] [timeout]" in square brackets. Be aware that putting things in square brackets in that context usually means that the argument is optional, which does not appear to be the intent here.

Use standard algorithms

The get_domains_from_file routine can be entirely replaced by a standard algorithm instead. In particular, instead of this:

std::string domains_file = argv[1];
std::vector<std::string> domains = get_domains_from_file(domains_file);

You could use this:

std::ifstream in{argv[1]};
std::vector<std::string> domains;
std::copy(std::istream_iterator<std::string>(in),
    std::istream_iterator<std::string>(), 
    std::back_inserter(domains));

Declare and initialize in one step

The code currently includes these two lines:

std::ofstream myfile;
myfile.open(output_file);

I would advocate combining them like this:

std::ofstream myfile{output_file};

Decompose the program into smaller parts

Right now, almost all of the code is in main which isn't necessarily wrong, but it means that it's not only hard to reuse but also hard to troubleshoot. Better is to separate the code into small chunks. It makes it both easier to understand and easier to fix or improve. In this case, I'd say that it would make sense to have a function which would take a domain name and return a bool indicating that the port is open or perhaps a non-empty string containing the address that was found to have the open port.

Don't use obsolete functions

The gethostbyname() function is obsolete as is inet_ntoa. Instead, you could either use getaddrinfo and inet_ntop or perhaps better, something like the boost asio library. Combining this suggestion with the previous one, we might write such a function like this:

std::string isPortOpen(const std::string &domain, const std::string &port)
{
    addrinfo *result;
    addrinfo hints{};    // aggregate initialization (no need for memset)
    hints.ai_family = AF_UNSPEC;   // either IPv4 or IPv6
    hints.ai_socktype = SOCK_STREAM; 
    char addressString[INET6_ADDRSTRLEN];  // big enough for IPv4 or IPv6
    const char *retval = nullptr;
    if (0 != getaddrinfo(domain.c_str(), port.c_str(), &hints, &result)) {
        return "";
    }
    for (addrinfo *addr = result; addr != nullptr; addr = addr->ai_next) {
        int handle = socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
        if (handle == -1) {
            continue;
        }
        if (connect(handle, addr->ai_addr, addr->ai_addrlen) != -1) {
            switch(addr->ai_family) {
                case AF_INET:
                    retval = inet_ntop(addr->ai_family, 
                        &(reinterpret_cast<sockaddr_in *>(addr->ai_addr)->sin_addr), 
                        addressString, INET6_ADDRSTRLEN);
                    break;
                case AF_INET6:
                    retval = inet_ntop(addr->ai_family, 
                        &(reinterpret_cast<sockaddr_in6 *>(addr->ai_addr)->sin6_addr), 
                        addressString, INET6_ADDRSTRLEN);
                    break;
                default:
                    // unknown family
                    retval = nullptr;
            }
            close(handle);
            break;
        }
    }
    freeaddrinfo(result);
    return retval==nullptr ? "" : retval;
}

This has several advantages over the existing code. First, it's considerably shorter and (at least to me!) easier to read and understand. Second, it does not use obsolete functions. Third, it can handle IPv4 or IPv6 with no modification to the code. Fourth, the code is re-entrant which will be important if you decide to make this multithreaded.

Use "range for" and simplify your code

Here is an alternative implementation of your program which uses the function posted above:

int main(int argc, char *argv[]) {
    if (argc != 6) {
        std::cerr << "Usage: " << argv[0] << " domains.txt port threads timeout output.txt\n";
        return 0;
    }

    std::string domains_file{argv[1]};
    std::string output_file{argv[5]};
    std::string port{argv[2]};
    int threads {atoi(argv[3])};
    int timeout {atoi(argv[4])};

    std::ifstream in{domains_file};
    std::vector<std::string> domains;
    std::copy(std::istream_iterator<std::string>(in),
        std::istream_iterator<std::string>(), 
        std::back_inserter(domains));

    std::ofstream myfile{output_file};

    // this is the range for syntax
    for (const auto &domain: domains) {
        std::string addr = isPortOpen(domain, port);
        if (addr.length()) {
            myfile << domain << ":" << addr << "\n";
        }
    }
    myfile.close();
}

Use std::future to simplify multithreading

If we want to try to do this with multithreading, it's actually quite simple with std::future. One way to do this would be to replace the for loop shown in main above with this:

std::vector<std::future<std::string>> results;
for (const auto &domain: domains) {
    results.push_back(std::async(isPortOpen, domain, port));
}
std::for_each(results.begin(), results.end(), 
    [&myfile](std::future<std::string>& f){myfile << f.get() << '\n';});

In this example, the code might or might not launch additional threads because this code doesn't specify the launch policy for std::async. One can easily add a launch policy as the first argument to std::async, but it probably doesn't make much difference in this case.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Putting it all together

Here's how to do it:

#include <iostream>
#include <fstream>
#include <vector>
#include <iterator>
#include <algorithm>
#include <string>
#include <future>
#include <cstring>
#include <sys/socket.h>
#include <netdb.h>
#include <arpa/inet.h>
#include <unistd.h>

std::string isPortOpen(const std::string &domain, const std::string &port)
{
    addrinfo *result;
    addrinfo hints{}; 
    hints.ai_family = AF_UNSPEC;   // either IPv4 or IPv6
    hints.ai_socktype = SOCK_STREAM; 
    char addressString[INET6_ADDRSTRLEN];
    const char *retval = nullptr;
    if (0 != getaddrinfo(domain.c_str(), port.c_str(), &hints, &result)) {
        return "";
    }
    for (addrinfo *addr = result; addr != nullptr; addr = addr->ai_next) {
        int handle = socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
        if (handle == -1) {
            continue;
        }
        if (connect(handle, addr->ai_addr, addr->ai_addrlen) != -1) {
            switch(addr->ai_family) {
                case AF_INET:
                    retval = inet_ntop(addr->ai_family, &(reinterpret_cast<sockaddr_in *>(addr->ai_addr)->sin_addr), addressString, INET6_ADDRSTRLEN);
                    break;
                case AF_INET6:
                    retval = inet_ntop(addr->ai_family, &(reinterpret_cast<sockaddr_in6 *>(addr->ai_addr)->sin6_addr), addressString, INET6_ADDRSTRLEN);
                    break;
                default:
                    // unknown family
                    retval = nullptr;
            }
            close(handle);
            break;
        }
    }
    freeaddrinfo(result);
    return retval==nullptr ? "" : domain + ":" + retval + "\n";
}

int main(int argc, char *argv[]) {
    if (argc != 6) {
        std::cerr << "Usage: " << argv[0] << " domains.txt port threads timeout output.txt\n";
        std::exit(-1);
    }

    std::string domains_file = argv[1];
    std::string output_file  = argv[5];
    std::string port         = argv[2];
    int threads              = atoi(argv[3]);
    int timeout              = atoi(argv[4]);

    std::ifstream in{argv[1]};
    std::vector<std::string> domains;
    std::copy(std::istream_iterator<std::string>(in),
        std::istream_iterator<std::string>(), 
        std::back_inserter(domains));

    std::ofstream myfile{output_file};

    std::vector<std::future<std::string>> results;
    for (const auto &domain: domains) {
        results.push_back(std::async(isPortOpen, domain, port));
    }
    std::for_each(results.begin(), results.end(), 
        [&myfile](std::future<std::string>& f){myfile << f.get();});

    myfile.close();
}

That's the whole thing, and it can be compiled with:

g++ -Wall -Wextra -pedantic -std=c++11 portchek.cpp -o portchek -lpthread

\$\endgroup\$
  • \$\begingroup\$ Nice answer @Edward. One more think, could you please edit your answer and add as a final step the whole code as you think it should look like after using all the given advices ? ^_^ I'm havin' trouble making it work \$\endgroup\$ – яүυк Feb 26 '17 at 12:01
  • \$\begingroup\$ thanks a lot. Apparently I didn't run this as you wrote :) Another question (answer only if you want: Is there any way of passing the number of threads int threads to this ? I can't find anything in the docs \$\endgroup\$ – яүυк Feb 26 '17 at 14:23
  • \$\begingroup\$ You could add std::launch::async as the first argument to each invocation of std::async and then launch them in groups of threads, but then you'd have to keep track of when each thread finishes to launch a new one. In other words, it's likely to be more effort than its worth. The plain old std::async is likely to be more efficient and easier to use. \$\endgroup\$ – Edward Feb 26 '17 at 14:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.