Fix critical bugs: DNS error handling, thread safety, input validation, and security improvements

README.md

@@ -1,13 +1,16 @@
 # PingDNS
 
-PingDNS is a command-line tool for mass pinging DNS servers to measure their latency using ICMP pings. It supports scanning up to 60,000 servers efficiently with multi-threading, configurable ping parameters, and optional geolocation data retrieval via GeoIP.
+PingDNS is a command-line tool for mass pinging DNS servers to measure their latency using ICMP pings. It supports scanning up to 100,000 servers efficiently with multi-threading, configurable ping parameters, enhanced error handling, and optional geolocation data retrieval via GeoIP.
 
 ## Features
 - Mass ping DNS servers (hardcoded and from public-dns.info)
-- Multi-threaded for high performance
-- Configurable ping count, timeout, and thread count
+- Multi-threaded for high performance with automatic thread count optimization
+- Configurable ping count, timeout, and thread count with input validation
+- Enhanced error handling and thread-safe logging
 - Geolocation support for server location data
+- Security improvements with input validation and safe file handling
 - Outputs results to console and `dns_results.csv`
+- Detailed error logging to `dns_errors.log`
 
 ## Installation
 1. Clone the repository:
@@ -45,7 +48,21 @@ Note: Use sudo or set CAP_NET_RAW capability:
 2. Scan 100 servers with 5 pings each, 500ms timeout, and 8 threads:
    sudo ./pingdns -n 100 -pings 5 -timeout 500 -t 8
 
-See the man page (docs/pingdns.1) for more examples.
+3. Parameters and validation:
+   - `-n <number>`: Number of servers to scan (1-100,000)
+   - `-pings <count>`: Number of pings per server (1-100, default: 3)
+   - `-timeout <ms>`: Ping timeout in milliseconds (1-60,000, default: 250)
+   - `-t <threads>`: Number of threads (default: CPU cores, max: 4x CPU cores)
+
+See the man page (docs/pingdns.1) for more examples and detailed documentation.
+
+## Security & Error Handling
+- Enhanced input validation prevents invalid parameters
+- Thread-safe logging with mutex protection
+- Improved error messages for debugging
+- Safe file handling with path validation
+- Detailed error logging to `dns_errors.log`
+- Graceful handling of permission issues and network failures
 
 ## Dependencies
 - libcurl

docs/pingdns.1

@@ -1,4 +1,4 @@
-.TH PINGDNS 1 "August 3, 2025" "Version 1.0" "User Commands"
+.TH PINGDNS 1 "September 8, 2025" "Version 1.1" "User Commands"
 .SH NAME
 pingdns - mass ping DNS servers to test latency and retrieve geolocation data
 
@@ -8,28 +8,33 @@ pingdns - mass ping DNS servers to test latency and retrieve geolocation data
 
 .SH DESCRIPTION
 .B pingdns
-is a command-line tool designed to perform mass ping scans on DNS servers to measure their latency using ICMP pings. It is capable of scanning a large number of DNS servers—up to 60,000—by leveraging multi-threading for high efficiency. The tool uses a hardcoded list of well-known DNS servers and can optionally augment this list by downloading additional servers from "https://public-dns.info/nameservers.txt". It also retrieves geolocation data for each server using the GeoIPCity database, providing details such as country and city when available.
+is a command-line tool designed to perform mass ping scans on DNS servers to measure their latency using ICMP pings. It is capable of scanning a large number of DNS servers—up to 100,000—by leveraging multi-threading for high efficiency and scalability. The tool uses a hardcoded list of well-known DNS servers and can optionally augment this list by downloading additional servers from "https://public-dns.info/nameservers.txt". It also retrieves geolocation data for each server using the GeoIPCity database, providing details such as country and city when available.
 
 The output includes the server name, IP address, geolocation (if available), and average round-trip time (RTT), displayed on the console and saved to a CSV file named
 .B dns_results.csv
 , sorted by RTT. The tool is robust, continuing to operate even if it fails to download the extended server list or geolocation database, falling back to the hardcoded list and omitting geolocation data as needed.
 
+.B pingdns
+features enhanced error handling, thread-safe logging, input validation, and security improvements. All errors are logged to
+.B dns_errors.log
+for debugging purposes.
+
 .B pingdns
 is ideal for network enthusiasts, researchers, or anyone needing to evaluate the performance of DNS servers on a large scale.
 
 .SH OPTIONS
 .TP
 \fB\-n\fR \fInumber\fR
-Limits the number of DNS servers to scan. If omitted, all available servers (hardcoded and downloaded) are scanned.
+Limits the number of DNS servers to scan (1-100,000). If omitted, all available servers (hardcoded and downloaded) are scanned. Numbers above 100,000 will generate a warning about memory usage.
 .TP
 \fB\-pings\fR \fIcount\fR
-Specifies the number of pings to send to each DNS server. Default is 3.
+Specifies the number of pings to send to each DNS server (1-100). Default is 3.
 .TP
 \fB\-timeout\fR \fIms\fR
-Sets the timeout for each ping in milliseconds. Default is 250 ms.
+Sets the timeout for each ping in milliseconds (1-60,000). Default is 250 ms.
 .TP
 \fB\-t\fR \fIthreads\fR
-Defines the number of threads to use for pinging. Defaults to the number of logical CPU cores.
+Defines the number of threads to use for pinging (1-max). Defaults to the number of logical CPU cores. Thread count is automatically capped to 4x CPU cores for optimal performance.
 
 .SH EXAMPLES
 Here are several practical examples demonstrating how to use
@@ -82,6 +87,28 @@ Scans 1000 DNS servers with 3 pings each, using 32 threads to handle the large w
 .TP
 \fBdns_results.csv\fR
 The output CSV file containing scan results, including server name, IP address, geolocation (if available), and average RTT.
+.TP
+\fBdns_errors.log\fR
+Error log file containing detailed information about failed DNS lookups, socket errors, and other issues encountered during scanning.
+
+.SH SECURITY CONSIDERATIONS
+.B pingdns
+requires raw socket privileges to send ICMP packets. Run with sudo or set the CAP_NET_RAW capability:
+.PP
+.B sudo setcap cap_net_raw+ep pingdns
+.PP
+The tool validates all input parameters and file paths to prevent security issues. Downloaded files are validated before processing.
+
+.SH EXIT STATUS
+.TP
+0
+Successful execution
+.TP
+1
+Invalid arguments, permission errors, or other failures
+
+.SH BUGS
+Report bugs to the project repository on GitHub.
 
 .SH AUTHOR
 Stefan Fröberg <haxbox2000@gmail.com>

icmp.h

@@ -18,14 +18,16 @@
 #include <thread>
 #include <csignal>
 #include <fstream>
+#include <mutex>
 
 namespace icmp_util {
 
     // Global flag for signal handling
     volatile sig_atomic_t interrupted = 0;
 
-    // Log file for errors
+    // Log file for errors with thread-safe access
     static std::ofstream log_file("dns_errors.log", std::ios::app);
+    static std::mutex log_mutex;
 
     // Compute checksum for ICMP packet
     static uint16_t compute_checksum(const void* data, size_t len) {
@@ -51,14 +53,24 @@ namespace icmp_util {
 
     // Send an ICMP Echo Request and wait for Echo Reply
     inline ping_result ping(const std::string& host, std::chrono::milliseconds timeout = std::chrono::milliseconds(250)) {
+        // Validate timeout range
+        if (timeout.count() < 1) {
+            timeout = std::chrono::milliseconds(1);
+        } else if (timeout.count() > 60000) {
+            timeout = std::chrono::milliseconds(60000);
+        }
         // Resolve host to IPv4 address
         struct addrinfo hints = {};
         hints.ai_family = AF_INET;
         hints.ai_socktype = SOCK_RAW;
         hints.ai_protocol = IPPROTO_ICMP;
         struct addrinfo* res;
-        if (getaddrinfo(host.c_str(), nullptr, &hints, &res) != 0) {
-            log_file << "getaddrinfo failed for " << host << ": " << gai_strerror(errno) << "\n";
+        int gai_error = getaddrinfo(host.c_str(), nullptr, &hints, &res);
+        if (gai_error != 0) {
+            {
+                std::lock_guard<std::mutex> lock(log_mutex);
+                log_file << "getaddrinfo failed for " << host << ": " << gai_strerror(gai_error) << std::endl;
+            }
             return {false, std::chrono::milliseconds(0)};
         }
         struct sockaddr_in* addr = reinterpret_cast<struct sockaddr_in*>(res->ai_addr);
@@ -67,7 +79,14 @@ namespace icmp_util {
         // Create raw ICMP socket
         int sock = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
         if (sock < 0) {
-            log_file << "socket creation failed for " << host << ": " << strerror(errno) << "\n";
+            {
+                std::lock_guard<std::mutex> lock(log_mutex);
+                log_file << "Raw socket creation failed for " << host << " (" << dest_ip << "): " << strerror(errno);
+                if (errno == EPERM) {
+                    log_file << " - Insufficient privileges (need CAP_NET_RAW or run as root)";
+                }
+                log_file << std::endl;
+            }
             freeaddrinfo(res);
             return {false, std::chrono::milliseconds(0)};
         }
@@ -90,7 +109,10 @@ namespace icmp_util {
         struct sockaddr_in dest_addr = *addr;
         auto send_time = std::chrono::high_resolution_clock::now();
         if (sendto(sock, packet.data(), PACKET_SIZE, 0, (struct sockaddr*)&dest_addr, sizeof(dest_addr)) < 0) {
-            log_file << "sendto failed for " << host << ": " << strerror(errno) << "\n";
+            {
+                std::lock_guard<std::mutex> lock(log_mutex);
+                log_file << "sendto failed for " << host << " (" << dest_ip << "): " << strerror(errno) << std::endl;
+            }
             close(sock);
             freeaddrinfo(res);
             return {false, std::chrono::milliseconds(0)};
@@ -121,7 +143,10 @@ namespace icmp_util {
                 ssize_t received = recvfrom(sock, recv_buf.data(), recv_buf.size(), 0,
                                           (struct sockaddr*)&from_addr, &from_len);
                 if (received < 0) {
-                    log_file << "recvfrom failed for " << host << ": " << strerror(errno) << "\n";
+                    {
+                        std::lock_guard<std::mutex> lock(log_mutex);
+                        log_file << "recvfrom failed for " << host << " (" << dest_ip << "): " << strerror(errno) << std::endl;
+                    }
                     close(sock);
                     freeaddrinfo(res);
                     return {false, std::chrono::milliseconds(0)};
@@ -139,7 +164,8 @@ namespace icmp_util {
                                 auto rtt = std::chrono::duration_cast<std::chrono::milliseconds>(
                                     recv_time - send_time);
                                 if (src_ip != dest_ip) {
-                                    log_file << "ICMP reply from wrong source for " << host << ": got " << src_ip << ", expected " << dest_ip << "\n";
+                                    std::lock_guard<std::mutex> lock(log_mutex);
+                                    log_file << "ICMP reply from unexpected source for " << host << ": got " << src_ip << ", expected " << dest_ip << std::endl;
                                 }
                                 close(sock);
                                 freeaddrinfo(res);
@@ -149,7 +175,10 @@ namespace icmp_util {
                     }
                 }
             } else if (sel < 0) {
-                log_file << "select failed for " << host << ": " << strerror(errno) << "\n";
+                {
+                    std::lock_guard<std::mutex> lock(log_mutex);
+                    log_file << "select failed for " << host << " (" << dest_ip << "): " << strerror(errno) << std::endl;
+                }
                 break;
             }
         }

pingdns.cpp

@@ -19,16 +19,21 @@
 #include <sys/stat.h>
 #include <iomanip>
 #include <unordered_set>
+#include <sys/wait.h>
+#include <unistd.h>
 
+/**
+ * DNS Server structure to hold server information and ping results
+ */
 struct dns_server {
-    std::string name;
-    std::string ip;
-    std::chrono::milliseconds avg_rtt;
-    std::chrono::milliseconds min_rtt;
-    std::chrono::milliseconds max_rtt;
-    bool success;
-    std::string country;
-    std::string city;
+    std::string name;      // Server name/provider
+    std::string ip;        // IPv4 address
+    std::chrono::milliseconds avg_rtt;  // Average round-trip time
+    std::chrono::milliseconds min_rtt;  // Minimum round-trip time
+    std::chrono::milliseconds max_rtt;  // Maximum round-trip time
+    bool success;          // Whether ping was successful
+    std::string country;   // Geolocation country
+    std::string city;      // Geolocation city
 };
 
 // Global progress counter
@@ -38,6 +43,11 @@ void signal_handler(int sig) {
     icmp_util::interrupted = 1;
 }
 
+/**
+ * Validates IPv4 address format
+ * @param ip IPv4 address string to validate
+ * @return true if valid IPv4, false otherwise
+ */
 bool is_ipv4(const std::string& ip) {
     static const std::regex ipv4_regex(R"(^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$)");
     std::smatch match;
@@ -142,16 +152,33 @@ bool download_file(const std::string& url, const std::string& outfile) {
 }
 
 bool unzip_file(const std::string& infile, const std::string& outfile) {
-    std::string cmd = "gzip -dc " + infile + " > " + outfile;
+    // Validate file paths to prevent directory traversal attacks
+    if (infile.find("..") != std::string::npos || outfile.find("..") != std::string::npos ||
+        infile.find(';') != std::string::npos || outfile.find(';') != std::string::npos ||
+        infile.find('|') != std::string::npos || outfile.find('|') != std::string::npos) {
+        std::cerr << "Error: Invalid characters in file path\n";
+        return false;
+    }
+
+    // Build command with properly quoted paths
+    std::string cmd = "/bin/gzip -dc '" + infile + "' > '" + outfile + "'";
     int ret = system(cmd.c_str());
     if (ret != 0) {
-        std::cerr << "Failed to unzip " << infile << "\n";
+        std::cerr << "Failed to decompress " << infile << " (exit code: " << ret << ")\n";
         return false;
     }
     return true;
 }
 
-// Modified ping_servers to accept configurable ping count and timeout
+/**
+ * Pings a range of DNS servers using ICMP
+ * @param servers Reference to vector of DNS servers
+ * @param start Starting index in the servers vector
+ * @param end Ending index in the servers vector
+ * @param server_count Total server count for progress reporting
+ * @param ping_count Number of pings per server
+ * @param timeout_ms Timeout in milliseconds per ping
+ */
 void ping_servers(std::vector<dns_server>& servers, size_t start, size_t end, size_t server_count, int ping_count, int timeout_ms) {
     for (size_t i = start; i < end && !icmp_util::interrupted; ++i) {
         auto& server = servers[i];
@@ -176,17 +203,32 @@ void ping_servers(std::vector<dns_server>& servers, size_t start, size_t end, si
 }
 
 bool get_geolocation(const std::string& ip, std::string& country, std::string& city) {
+    // Validate IP address format before processing
+    if (!is_ipv4(ip)) {
+        country = "Unknown";
+        city = "";
+        return false;
+    }
+
     GeoIP* gi = GeoIP_open("GeoIPCity.dat", GEOIP_MEMORY_CACHE);
     if (!gi) {
-        std::cerr << "Failed to open GeoIPCity.dat\n";
-        return false;
+        // Try alternative path or provide more specific error
+        gi = GeoIP_open("/usr/share/GeoIP/GeoIPCity.dat", GEOIP_MEMORY_CACHE);
+        if (!gi) {
+            country = "Unknown";
+            city = "";
+            return false;
+        }
     }
 
     GeoIPRecord* record = GeoIP_record_by_addr(gi, ip.c_str());
     bool found = false;
     if (record) {
-        country = record->country_name ? record->country_name : "Unknown";
-        city = record->city ? record->city : "";
+        // Safe string handling for potentially null fields
+        country = (record->country_name && strlen(record->country_name) > 0) ? 
+                  record->country_name : "Unknown";
+        city = (record->city && strlen(record->city) > 0) ? 
+               record->city : "";
         found = true;
         GeoIPRecord_delete(record);
     } else {
@@ -245,15 +287,22 @@ int main(int argc, char* argv[]) {
         if (arg == "-n" && i + 1 < argc) {
             try {
                 total_to_scan = std::stoul(argv[++i]);
+                if (total_to_scan == 0) {
+                    std::cerr << "Error: -n must be greater than 0\n";
+                    return 1;
+                }
+                if (total_to_scan > 100000) {
+                    std::cerr << "Warning: Scanning more than 100,000 servers may consume significant memory and time\n";
+                }
             } catch (const std::exception& e) {
                 std::cerr << "Error: Invalid number for -n: " << argv[i] << "\n";
                 return 1;
             }
         } else if (arg == "-pings" && i + 1 < argc) {
             try {
                 ping_count = std::stoi(argv[++i]);
-                if (ping_count < 1) {
-                    std::cerr << "Error: -pings must be at least 1\n";
+                if (ping_count < 1 || ping_count > 100) {
+                    std::cerr << "Error: -pings must be between 1 and 100\n";
                     return 1;
                 }
             } catch (const std::exception& e) {
@@ -263,8 +312,8 @@ int main(int argc, char* argv[]) {
         } else if (arg == "-timeout" && i + 1 < argc) {
             try {
                 timeout_ms = std::stoi(argv[++i]);
-                if (timeout_ms <= 0) {
-                    std::cerr << "Error: -timeout must be positive\n";
+                if (timeout_ms <= 0 || timeout_ms > 60000) {
+                    std::cerr << "Error: -timeout must be between 1 and 60000 ms\n";
                     return 1;
                 }
             } catch (const std::exception& e) {
@@ -278,12 +327,23 @@ int main(int argc, char* argv[]) {
                     std::cerr << "Error: -t must be at least 1\n";
                     return 1;
                 }
+                // Cap thread count to reasonable limits
+                unsigned int max_threads = std::thread::hardware_concurrency() * 4;
+                if (max_threads == 0) max_threads = 16; // fallback
+                if (num_threads > max_threads) {
+                    std::cerr << "Warning: Thread count capped to " << max_threads << " (4x CPU cores)\n";
+                    num_threads = max_threads;
+                }
             } catch (const std::exception& e) {
                 std::cerr << "Error: Invalid value for -t: " << argv[i] << "\n";
                 return 1;
             }
         } else {
             std::cerr << "Usage: " << argv[0] << " [-n <number>] [-pings <count>] [-timeout <ms>] [-t <threads>]\n";
+            std::cerr << "  -n <number>    Number of servers to scan (1-100000)\n";
+            std::cerr << "  -pings <count> Number of pings per server (1-100, default: 3)\n";
+            std::cerr << "  -timeout <ms>  Ping timeout in milliseconds (1-60000, default: 250)\n";
+            std::cerr << "  -t <threads>   Number of threads to use (default: CPU cores)\n";
             return 1;
         }
     }