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:
- The exit code doesn't yield any information, apart from "it worked" or "it failed"
- 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:
- Getting the address info for a domain name.
- 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.
