-
Notifications
You must be signed in to change notification settings - Fork 260
New option --traceroute to send a traceroute with fping #451
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
base: develop
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @gsnw-sebast, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates a new traceroute capability into the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a --traceroute option to fping, which is a significant and useful feature. The implementation correctly handles both IPv4 and IPv6, including the necessary ICMP message processing and TTL manipulation. The code is generally well-structured and follows the existing patterns in the codebase. I've provided a few suggestions to improve maintainability by reducing code duplication, enhance efficiency by optimizing memory allocation, and increase correctness in option parsing. Overall, this is a solid contribution.
|
The code block for traceroute in the function Update: Optimization completed |
73d0936 to
15bc7d4
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new --traceroute feature to fping. The implementation is comprehensive, covering both IPv4 and IPv6, and handling various fping options. However, I've found several issues that need to be addressed. There is a critical race condition when using traceroute with multiple hosts due to improper handling of socket options. I've also identified a high-severity buffer over-read vulnerability and a bug that affects IPv6 traceroute output. Additionally, there are several opportunities to improve code quality by removing hardcoded values and refactoring duplicated code.
c013498 to
c450285
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new --traceroute feature, which is a great addition to fping's capabilities. The implementation is mostly complete and handles both IPv4 and IPv6. I've identified a few issues related to option parsing, the logic for the traceroute loop, and error handling that should be addressed to ensure correctness and robustness. Specifically, the check for the number of hosts in traceroute mode is currently ineffective, there's an off-by-one error in the traceroute loop, a potential infinite loop on the last hop, and the error handling for setsockopt could be strengthened. The documentation and general structure of the changes are well done.
| if (traceroute_flag) { | ||
| printf("%s: hop %d no reply\n", h->host, h->trace_ttl); | ||
| h->trace_ttl++; | ||
| if (h->trace_ttl > (int)ttl) h->trace_ttl = (int)ttl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line, which clamps h->trace_ttl to ttl, can cause an infinite loop. When the main loop condition is corrected to h->trace_ttl <= (int)ttl, if the last hop (ttl) times out, h->trace_ttl is incremented to ttl + 1 on the previous line, then clamped back to ttl here. The next ping event will then re-send a ping for the same hop ttl, leading to an infinite loop. This clamp should be removed, as the loop termination is correctly handled by the scheduler condition.
| if (h->trace_ttl > (int)ttl) { | ||
| h->trace_ttl = (int)ttl; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block, which clamps h->trace_ttl to ttl, can cause an infinite loop. When the main loop condition is corrected to h->trace_ttl <= (int)ttl, if the last hop (ttl) does not reach the destination, h->trace_ttl is incremented to ttl + 1, then clamped back to ttl here. The next ping event will then re-send a ping for the same hop ttl, leading to an infinite loop if that hop also doesn't reach the destination. This clamp should be removed.
| if (setsockopt(socket4, IPPROTO_IP, IP_TTL, &ttl_set, sizeof(ttl_set))) | ||
| perror("setsockopt IP_TTL"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing to set the IP_TTL socket option is a fatal error for traceroute mode, as it won't be able to control the TTL of outgoing packets. The program should exit with an error instead of just printing to perror and continuing, which would lead to incorrect traceroute behavior.
if (setsockopt(socket4, IPPROTO_IP, IP_TTL, &ttl_set, sizeof(ttl_set)))
errno_crash_and_burn("setsockopt IP_TTL");| if (setsockopt(socket6, IPPROTO_IPV6, IPV6_UNICAST_HOPS, &ttl_set, sizeof(ttl_set))) { | ||
| perror("setsockopt IPV6_UNICAST_HOPS"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing to set the IPV6_UNICAST_HOPS socket option is a fatal error for traceroute mode, as it won't be able to control the hop limit of outgoing packets. The program should exit with an error instead of just printing to perror and continuing, which would lead to incorrect traceroute behavior.
if (setsockopt(socket6, IPPROTO_IPV6, IPV6_UNICAST_HOPS, &ttl_set, sizeof(ttl_set))) {
errno_crash_and_burn("setsockopt IPV6_UNICAST_HOPS");
}| /* Loop and count mode: schedule next ping */ | ||
| if (loop_flag || (count_flag && event->ping_index + 1 < count)) { | ||
| /* Loop, count and traceroute mode: schedule next ping */ | ||
| if (loop_flag || (count_flag && event->ping_index + 1 < count) || (traceroute_flag && h->trace_ttl < (int)ttl)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop condition for traceroute, h->trace_ttl < (int)ttl, is an off-by-one error. If a user specifies --ttl 30, they expect fping to try all hops from 1 to 30. With the current condition, it will only try up to TTL 29. The condition should be h->trace_ttl <= (int)ttl to include the final hop.
if (loop_flag || (count_flag && event->ping_index + 1 < count) || (traceroute_flag && h->trace_ttl <= (int)ttl)) {|
I appreciate the effort, but I am a bit sceptical about this change. It adds quite a bit of complexity, and I don't yet understand why it would be a useful addition to fping. In particular, the limitation of only allowing one host seems to defeat the purpose of adding this to fping, which is focused on doing parallel requests to multiple hosts. |
|
I wouldn't have expected such complexity, and querying different hosts doesn't make it any easier. I would have no problem closing the function change or, if necessary, creating something like ftraceroute, i.e., an extra program based on fping outside of fping itself. Especially since the tests are currently very complex |
|
I also appreciate the effort, but it seems to me as if this functionality does not really fit into the existing fping code. Please note that issue #71 explicitly asks for parallel traceroutes to multiple hosts. |
|
I took another look at the whole thing, and if sendmsg() is used instead of sendto(), a parallel query should be possible. I've now made the change. The output is still a bit ugly, but it works. |
The traceroute function is now complete. There is no JSON output in the current version, and testing is still pending, if it is even possible given the required root privileges.
IPv6 also works.
./fping --traceroute 8.8.8.8Output
Test Checklist