4
\$\begingroup\$

I made a ping sweep for the first time, and I want to do better. What do you think that I might be missing?

#! /usr/bin/env python3

import ipaddress
import platform
import socket
import subprocess
import time
from concurrent.futures import ThreadPoolExecutor
from itertools import repeat

TIME_OUT = 3


def get_local_network():
    """Returns local IP,subnet mask and
    other network informations"""

    hostname = socket.gethostname()
    s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
    s.settimeout(TIME_OUT)
    s.connect(("8.8.8.8", 80))
    ip = s.getsockname()[0]
    s.close()

    os_name = platform.system().lower()

    mask = None
    if "windows" in os_name:
        output = subprocess.check_output("ipconfig", text=True)
        for line in output.splitlines():
            if ip in line:
                continue
            if "Subnet Mask" in line or "Alt Ağ Maskesi" in line:
                mask = line.split(":")[1].strip()
                break
    else:
        output = subprocess.check_output(
            f"ip addr show | grep '{ip}'", shell=True, text=True
        )
        line = output.strip()
        mask = line.split()[1].split("/")[1]

    net = ipaddress.ip_network(f"{ip}/{mask}", strict=False)

    return {
        "hostname": hostname,
        "host_ip": ip,
        "subnet_mask": mask,
        "network_ip": str(net.network_address),
        "broadcast": str(net.broadcast_address),
        "platform": os_name,
    }


def icmp_ping(ip, subnet_mask):
    """
    Pings specified IP address.
    If ping successfull prints IP.
    """
    try:
        command = '-n' if platform.system().lower() == "windows" else '-c'

        result = subprocess.call(
            ["ping", command, "1", ip],
            stderr=subprocess.DEVNULL,
            stdout=subprocess.DEVNULL,
        )
        if result == 0:
            print(f"[+] Acitve: {ip}/{subnet_mask}")

    except OSError as e:
        print(f"An Error Occured: {e}")
    except ValueError as e:
        print(f"An Error Occured: {e}")
    except PermissionError as e:
        print(f"An Error Occured: {e}")
    except FileNotFoundError as e:
        print(f"An Error Occured: {e}")


def threader(net_ip, subnet_mask):
    """
    Makes IP list for given IP and subnet mask.
    And with ThreadPoolExecutor pings all IP's.
    """
    ip_count = 32 - int(subnet_mask)

    total_ip = (2**ip_count) - 2

    base_ip = net_ip.rsplit(".", 1)[0]

    ip_list = [f"{base_ip}.{i}" for i in range(1, total_ip + 1)]
    with ThreadPoolExecutor(max_workers=40) as executor:
        executor.map(icmp_ping, ip_list, repeat(subnet_mask))


def main():
    info_package = get_local_network()

    print(f"HOST:{info_package['hostname']}")
    print(f"Host IP:{info_package['host_ip']}")
    print(f"Network IP Address:{info_package['network_ip']}")
    print(f"OS:{info_package['platform']}")
    threader(info_package["network_ip"], info_package["subnet_mask"])
    print("IP Scan Finished. Glad you were here :)")


main()
New contributor
Kaan Aral is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
3
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ Commented yesterday
  • \$\begingroup\$ I am not very familiar with python, and I have not taken the time to review the code. I would not be able to help with that, and there are experts here. I suggest, though that the proper tool for this is 'nmap. Note that scanning networks without permission can get you in trouble. \$\endgroup\$ Commented yesterday
  • \$\begingroup\$ A failure to "reply to ping" does not mean there's nothing there. Be aware this will miss hosts which don't answer ICMP ping requests. \$\endgroup\$ Commented yesterday

3 Answers 3

4
\$\begingroup\$

It's not a bad idea to have a main function, but you haven't really gained anything if you're not guarding the call of it, as main() will still be evaluated any time the module is imported.

if __name__ == '__main__':
    main()

Also:

        command = '-n' if platform.system().lower() == "windows" else '-c'

To generalize this, in case you have other platforms/systems with different options, you might have an options_map, leveraging dict.get to provide a default if the platform/system does not have a specified option.

options_map = {
    'windows': '-n'
}

Now you can write:

        command = options_map.get(platform.system().lower(), '-c')

For the following loop, we might replace this logic with a generator expression and a call to next. We can also make the check for "Subnet Mask" or "Alt Ağ Maskesi" in each line more extensible using any.

        output = subprocess.check_output("ipconfig", text=True)
        for line in output.splitlines():
            if ip in line:
                continue
            if "Subnet Mask" in line or "Alt Ağ Maskesi" in line:
                mask = line.split(":")[1].strip()
                break

Suggested:

        output = subprocess.check_output("ipconfig", text=True)
        mask = next((
            line.split(":")[1].strip()
            for line in output.splitlines()
            if ip not in line
            if any(txt in line for txt in ["Subnet Mask", "Alt Ağ Maskesi"])
        ))
\$\endgroup\$
1
  • \$\begingroup\$ Oh that makes sense because every time I had to pass or over use that method again again. thank you \$\endgroup\$ Commented yesterday
3
\$\begingroup\$

Main guard

It is common to add a "main" guard:

if __name__ == '__main__':
    main()

Documentation

It is great that you have docstrings for your functions. It would also be good to describe the input and return types.

Also consider using type hints to describe input and return types to make the code more self-documenting.

The PEP 8 style guide also recommends adding a docstring to the top of the code to summarize its purpose. It would also be helpful to describe the output.

Import

The ruff tool identifies this line as unused:

import time

It can be deleted.

Portability

The Unicode characters in the source code cause a syntax error in my Linux environment. This is in the if "windows" clause. They also don't render well in my editor.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for reply I will fix what you said. \$\endgroup\$ Commented yesterday
  • \$\begingroup\$ @KaanAral: I'm glad to help. Another way to say "thanks" is to upvote all answers you find helpful, and to Accept one. \$\endgroup\$ Commented yesterday
2
\$\begingroup\$

function's promises

def get_local_network():
    """Returns local IP, subnet mask and
    other network informations"""

This is OK, but much less helpful than it might be.

An app author has to read down to the return statement to learn that a dict will be returned, with certain keys. Consider mentioning dict in the narrative prose, or better, consider annotating the signature:
def get_local_network() -> dict[str, str]:
Consider creating a class NetworkInfo: @dataclass, which would make the field details self documenting.

Some hosts have several interfaces beyond lo0 loopback, for example eth0, eth1, wi-fi wlan0, maybe a VPN tunnel interface. Mentioning "local IP" is a little on the vague side; consider mentioning that it's the "internet facing IP". Binding your temp socket to dns.google essentially asks "which local address matches the default-route interface?".

IPv4, IPv6

% host dns.google
dns.google has address 8.8.8.8
dns.google has address 8.8.4.4
dns.google has IPv6 address 2001:4860:4860::8888
dns.google has IPv6 address 2001:4860:4860::8844

There are three types of internet hosts:

  • IPv6 only
  • IPv6 + IPv4
  • IPv4 only

Given the "8.8.8.8" hardcode, the OP code does not support that first type.

portability

I will just note in passing that the OP code did not attempt to run ifconfig on my Mac, and died when the child reported that ip is not in ${PATH}. But sure, I understand that one cannot port to everything, especially platforms that you cannot easily access for testing.

NAT

It's common that a host is on (unroutable) network 10, will get to the internet via NAT, and internet webservers will see some different publicly routable IP address. We can learn the details via services like https://www.showmyip.com (which shows both IPv4 & IPv6 addresses), and https://ipinfo.io/json . The OP code doesn't go there, sticking to just local addresses, which is fine.

design of Public API

The main() caller ignores host_ip in favor of network_ip, and it's unclear what distinguishes them. Certainly it does not reveal what's seen on the other side of a NAT. Consider sticking to just network_ip. It reduces your documentation and maintenance burden.

wrong name

    command = '-n' if platform ... == "windows" else '-c'

That's an "option", or maybe a "parameter", not a "command".

Identifier names are important; they affect how we reason about the code. Name a spade a spade.

In a related vein, you use mask to refer to a mask_len in the range 0 .. 32. In contrast an IPv4 "mask" is a 32-bit quantity, typically written as a dotted quad. That said, the OP usage is still nice enough, though it would be much clearer if the mask type was consistently int and was annotated that way.

nit, typo: "successfull"

nit, typo: "Acitive"

indent

The v1 OP code used crazy one-space indent. Don't do that. The python interpreter can get by with that, since the source has unambiguous semantics. But your intent is to communicate technical ideas to humans, and we need more separation on those for and if clauses. Use black routinely to put your code back into canonical form, with uniform four-space indent.

unit tests

There aren't any.

But worse, you left off the if __name__ == "__main__": guard that we customarily include. By unconditionally running main(), you're making it impossible for a team member to write an automated test of the OP ping.py module by doing import ping. As written, the OP code would have the undesirable side effect of blasting the network with pings, even if the unit test just wanted to verify the spelling of the local OS platform.

error handler

    except OSError as e:
        print(f"An Error Occured: {e}")
    except ValueError as e:
        print(f"An Error Occured: {e}")
    except PermissionError as e:
        print(f"An Error Occured: {e}")
    except FileNotFoundError as e:
        print(f"An Error Occured: {e}")

It's not clear this is a big improvement over a default stack trace. Maybe delete the try: entirely?

Consider casting a wider net with except Exception as e:

If you like these four, then at least use modern syntax to combine them:
except (OSError, ValueError, PermissionError, FileNotFoundError) as e:. Well, maybe two of them; notice that FNF is child of OSError. And so is PermissionError.

BTW, kudos on using itertools for repeat(subnet_mask).

amplification

I'm not quite sure if your code focuses on pinging /32 hosts, or pinging broadcast addresses. But please understand that pinging 255.255.255.255 or similar addresses will produce N replies for 1 stimulus packet, and that's a recipe for packet queuing and packet loss within core devices like WAPs and routers. With TCP the end hosts monitor queue depth, for example by noticing packet loss events. They will try to measure available bandwidth and not exceed it, via flow control (the congestion window). Amplification attacks via broadcast ICMP, or UDP port 53, or via other vectors, will consume core buffers without regard to flow control.

If only /32's are of interest, then it's unclear why icmp_ping() accepts that 2nd mask argument.

fping

Forking forty ping processes seems slightly crazy, given that fping can do a fine job of managing many many more outstanding ICMP requests.

Unlike TCP's "app to app" nature, ICMP is a "host to host" protocol. The ping program puts a random message in the payload, discards any non-matching payloads that we overhear, and then prints an RTT round trip time. You're asking each instance of the program to discard 39 "noise" packets for each "signal" packet it receives. Switch to fping, and with low {memory, CPU} usage you'll be able to do big sweeps much faster.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback, I will fix all of them. \$\endgroup\$ Commented yesterday

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.