Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive results #2

Open
cristiancundari opened this issue Jan 23, 2025 · 8 comments
Open

False positive results #2

cristiancundari opened this issue Jan 23, 2025 · 8 comments
Assignees

Comments

@cristiancundari
Copy link

Hi, I just noticed that to check if a ping was successful it is not enough to verify that ping_result.returncode == 0
Sometimes the gateway replay to pinging outside the network with "Destination unreachable". In this case the returncode is still 0 because it received a packet back but from the gateway and not from the actual host.

Here I found a related thread on stackoverflow

I think to solve the issue we can change the success check to something like:
if ping_result.returncode == 0 and "byte=" in ping_result.stdout.decode("utf-8"):

So that it will check the presence of the text "byte=" in the ping output or something along that line

@zero-stroke
Copy link
Owner

zero-stroke commented Jan 23, 2025

Did this actually happen to you or is it just a potential issue? And I tried applying your fix but, at least on windows 11, it just resulted in only outages, returning false each time.

@cristiancundari
Copy link
Author

Yeah it happened to me.
Following an example; I wrote this simple code to demostrate that the returncode is also 0 when any packet is getting back no matter if it is the host replying or some other node in between:

import subprocess

def ping(host):
    command = ['ping', '-n', '4', host]
    result = subprocess.run(command, stdout=subprocess.PIPE,
                            stderr=subprocess.PIPE, text=True)
    if result.returncode == 0:
        print(f"Ping successful:\n{result.stdout}")
    else:
        print(f"Ping failed:\n{result.stderr}")

ping("9.9.9.9")

Executing this code I got: (I have italian language)

Image


I considered checking if the stdout contains the text byte= as a way to confirm successful pings, since that text appears only in such cases. However, I just found out that this behavior depends on the user's locale.

Another way to fix it could be to write a regex to search for the milliseconds number indicating the time. The time will be present only on successful pings, but ❗️I don't know if it is applicable to all possible locales and user systems❗️.

I was thinking something like this:

import re

# Regular expression pattern to match the response times in the ping output
# Looking for patterns like "<1ms", "=10ms", "<1 ms", "=10 ms", "<1.0ms", "=10.23ms", "<1.0 ms", "=10.23 ms"
success_pattern = re.compile(
    r"(?:<|=)\d+(?:\.\d+)? ?ms")

if ping_result.returncode == 0 and success_pattern.search(ping_result.stdout.decode("utf-8")):

@zero-stroke
Copy link
Owner

There is surely a more elegant way to test for a valid internet connection than regex, I'll have to look into it

cristiancundari added a commit to cristiancundari/uptime that referenced this issue Jan 24, 2025
- Replaced subprocess with icmplib to ping the hosts. This should fix issue zero-stroke#2 
- Improved handling of text alignment for better consistency.
- Replaced the timeout icon with a single-character Unicode emoji.
- Resolved an issue in average calculation where a division by zero could occur if no pings in the time interval were successful.
@zero-stroke
Copy link
Owner

I just changed the patch-1 branch to use socket instead of icmplib, this will probably fix the problem because it is no longer just a naive subprocess response code check. This also lets the script remain independent of outside dependencies. Let me know if this version works for you. I have only tested this on linux and windows 11, but not windows 10 or mac.

@cristiancundari
Copy link
Author

I tried this, but unfortunately, it doesn’t work for me. Here’s what I’m seeing on Windows 11:

Image

It seems the issue might be related to Quad9 and Google DNS not accepting connections on port 80, whereas Cloudflare does, likely because it has a web server running on that port. To verify, I manually tested port 80 on all three hosts using telnet, and only Cloudflare accepted the connection.
I’m not sure why it works for you, though

@zero-stroke
Copy link
Owner

That is weird, do other ports work for you, like maybe 443, 8080, or 53? Also, try increasing the timeout_limit to something higher like 5.

@cristiancundari
Copy link
Author

Port 443 and 53 work for all three DNS servers, but they don’t work for local network hosts since those hosts typically don’t listen on these ports.

It might be possible to use the same socket library to establish a connection using the ICMP protocol, but maybe that would require administrative privileges which isn't that good. I would try something tomorrow and keep you updated

@zero-stroke
Copy link
Owner

Ok interesting. Some logic to check multiple ports could be simplest.

@zero-stroke zero-stroke self-assigned this Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants