Add tries to scan_socket

[eiffel-fl]

Hi.

I added a number of tries to scan_socket, by default the number of try is 1 but it can be set with command line option --tries.
This should close #38.

I am not a Rust expert, so please let me know if I wrote something wrong or not elegant.
Moreover, I am not sure of my code for the Config part, so this part needs to be reviewed.

Best regards.

[niklasmohrin]

Sweet! I made some suggestions that I think would make the code a bit more clear. Especially the semantics around the ret variable seem a bit off to me. However, as I haven't contributed much to this repository yet, I do not insist that these changes are implemented and I don't know what is best used here. Anyway, good work and thanks for contributing!

[eiffel-fl]

You are welcome and thank you for you review!
About the ret variable, I am more used to C so I sometimes use C habit in other languages particularly when I do not know well like Rust.
I will try to remove it though but there are some points that I did not understand in your reviews.

[niklasmohrin]

Don't get me wrong, it is a valid way to solve the problem and you did a great job overall, but I personally find this pattern very iritating as it is more complex than what it accomplishes (see that one (somehwat long) comment where I try to explain my point).

[eiffel-fl]

There is not problem at all.
We are here to discuss and find the best solution, moreover I do agree that my solution makes the code less readable and I also have to adapt to the language.
If I understand correctly Rust philosophy the less mut variable you have the happier you are.

[bergabman]

Awesome PR! We are in the need of this functionality so it's a great PR to have. I think I know what @niklasmohrin tries to explain and I have to agree, so I downloaded this branch to test it. It does what it supposed to do, but that error on the top of the function does look a little weird. I think it also costs some memory allocation and reallocation because of mutability and reassignements. Based on your PR, there were just a few things I had to change to avoid extra allocations, and only allocate that error when and if it's needed. Please check the code I sketched up @eiffel-fl @niklasmohrin and let me know what you think. This would be a bit more rustacian style I think :)

    async fn scan_socket(&self, socket: SocketAddr) -> io::Result<SocketAddr> {
        for nr_try in 0..self.tries {
            match self.connect(socket).await {
                Ok(x) => {
                    debug!(
                        "Connection was successful, shutting down stream {}",
                        &socket
                    );
                    match x.shutdown(Shutdown::Both) {
                        Err(e) => debug!("Shutdown stream error {}", &e),
                        _ => {}
                    }
                    if !self.greppable {
                        if self.accessible {
                            println!("Open {}", socket.to_string());
                        } else {
                            println!("Open {}", socket.to_string().purple());
                        }
                    }
                    return Ok(socket);
                }
                Err(e) => {
                    if nr_try < self.tries {
                        continue;
                    } else {
                        match e.kind() {
                            ErrorKind::Other => {
                                if e.to_string().contains("No route to host")
                                    || e.to_string().contains("Network is unreachable")
                                {
                                    debug!("Socket connect error: {} {}", &e.to_string(), &socket);
                                    return Err(e);
                                } else {
                                    debug!("Socket connect error: {} {}", &e.to_string(), &socket);
                                    panic!("Too many open files. Please reduce batch size. The default is 5000. Try -b 2500.");
                                }
                            }
                            _ => return Err(e),
                        }
                    }
                }
            }
        }
        Err(io::Error::new(
            io::ErrorKind::Other,
            format!("Did not connect to {}", &socket),
        ))
    }

One interesting thing, maybe there is a problem with this logic, because no matter what I tried and made sure to cover all the arms inside the loop to or return or continue, the compiler kept complaining that not all the arms return a Result. So I added the last line as Err and it gets hit with the default tries. So it's something I don't get right now. Someone?

[niklasmohrin]

@bergabman You need to add semicolons after your match blocks (both the outer one and the one over e.kind()). (<- Nope that's not it)

Your loop is going from 0 to self.tries - 1. Therefore, the condition nr_try < self.tries is always true and you always continue. Without the Err after the loop, your function would not return anything if no connection succeeded. Furthermore, now that that is there, you always return that error instead of the one from inside the match arm (because the outer if condition is always true).

The logic is not right though, you still want to look at the error in every iteration and panic if needed. The check should only include the return statements. This is roughly how I imagine it:

async fn scan_socket(&self, socket: SocketAddr) -> io::Result<SocketAddr> {
    for nr_try in 1..=self.tries {
        match self.connect(socket).await {
            Ok(x) => {
                debug!(
                    "Connection was successful, shutting down stream {}",
                    &socket
                );
                match x.shutdown(Shutdown::Both) {
                    Err(e) => debug!("Shutdown stream error {}", &e),
                    _ => {}
                }
                if !self.greppable {
                    if self.accessible {
                        println!("Open {}", socket.to_string());
                    } else {
                        println!("Open {}", socket.to_string().purple());
                    }
                }
                return Ok(socket);
            }
            Err(e) => {
                // let error_string = e.to_string();
                if e.kind() == ErrorKind::Other {
                    if e.to_string().contains("No route to host")
                            || e.to_string().contains("Network is unreachable") {
                        debug!("Socket connect error: {} {}", &e.to_string(), &socket);
                    } else {
                        debug!("Socket connect error: {} {}", &e.to_string(), &socket);
                        panic!("Too many open files. Please reduce batch size. The default is 5000. Try -b 2500.");
                    }
                }
                
                if nr_try == self.tries {
                    return Err(io::Error::new(io::ErrorKind::Other, e.to_string()));
                }
            }
        };
    }
    
    // Only runs if self.tries is 0
    Err(io::Error::new(
        io::ErrorKind::Other,
        format!("Did not connect to {}", &socket),
    ))
}

Note that this does not include the caching of the result of .to_string(). Interestingly. this result may never be used, but I think that it is still best to always compute it since most times the error will be the network error. If that's not the case, we might have to come up with something more clever.

[bergabman]

I see. I was not sure in the 0..self.tries part but you are right, as we already start with 1 we can use that and inclusive ..= range so it's easier to compare. It's better. But where I match Error::kind, I think I cover everything, as all the errors have kind and if it's not Other, it should hit _. But if I always continue, that doesn't really matter :).
How you wrote it is the way to catch the too many files open exception and panic on it right away (current logic), instead of looping further. It would only cost 2 more loop iterations and it would probably hit panic, but your way seems the most optimal/efficient. 👍

[eiffel-fl]

I updated my code to take into account all reviews and discussions!
So please, let me know if there is a problem.

Moreover, can someone please reviews the code about reading into the config? I am not sure if tries is correctly taken into account.

I also add a print with eprintln to indicate to user that he/she sets tries to 0 so the code sets it to 1.
I would have liked to use warning! from tui but I had troubles with rust import system...

@bergabman I quickly checked your code and I agree with @niklasmohrin about the if condition.
About the panic, I would suggest that if we have to panic we should do it as soon as possible, but maybe panic here is not as severe as a kernel panic.

[niklasmohrin]

Awesome, we are getting there! Apart from a small suggestion from cargo clippy, the Ok arm looks perfect to me. I suggested a refactor in the Err arm, but it's mostly a syntax change with a slight performance improvement, as the logic was impeccable ✨ .

While reading your code that corrects the value of tries to 1 if 0 was given, I remembered that there is NonZeroU8 etc. If you were to use that, you could get rid of the Err after the for loop, because then you know that there will be at least one iteration (and that the last iteration will return). Note that the compiler might not figure that out by itself. If you get an error about a missing return after the loop, you just put unreachable!() after the loop to tell the compiler just that.

I have not come around to look at the config part yet, because I will have to read some documentation for that too and it is getting late. Maybe I will review tests and config tomorrow.

That said, great work and thank you for being so cooperative! I hope you don't take my large requests amiss and keep on writing great (Rust) code! 🦀

[bergabman]

That panic is a crash, but not as serious as a kernel panic for sure :D. We will handle that logic different in the future.
I also agree with that if statement there, so I think the structure/logic is good like this!

Those macros are exported to the root of the crate, so you can use them with

use super::warning;

and then

warning!("somestring");

To get Options and Config merged right, please add the tries to the input.rs merge_required! macro.
merge_required!(addresses, greppable, accessible, batch_size, tries, timeout, scan_order, command);

I will ask @bernardoamc to take a look at it as well, but I also think we are getting there and it will be ok for a merge. And we have to change the README and the manual so people will know about the function. :D

[bernardoamc]

First thank you so much for the contribution. <3

I have a proposal that might change the code a bit.

Proposal:

I think we could simplify a lot this logic by just retrying the scanner itself inside main.rs. It would go like this:

1. Create a set inside main.
2. Run the scanner
3. Store open ports within set
4. Go to step 1 until retries are exhausted.

Benefits:

1. Keep the Scanner with a single responsibility
2. Small change required, so the code becomes more intuitive

Downsides

1. We might scan open ports multiple times

I don't mind the downside that much since usually the majority of ports are closed, so we will scanner pretty much the same number of ports during every iteration.

cc/ @bergabman @niklasmohrin @eiffel-fl

[eiffel-fl]

@bergabman Thank you for the tip to include warning! I try a lot of combination but not this simple one...
I also modify the README.md on my local version, but where is the manual you are talking about? So I will update it too.

@bernardoamc You are welcome!
If the downside is not so important why not.
I did not go to a solution like this when I implemented the first version because I though RustScan goal was to be as performant as possible.
I will let others express their opinion about your suggestion.

[niklasmohrin]

@bernardoamc Wow, that approach is tempting indeed, this would be a very simple addition 😅

However, I would prefer the current approach as it is (probably) more performant and the condition that determines "port open" vs. "port closed" is in one place.

Furthermore, it allows for more specialized additions down the road (for example different number of tries allowed for different IPs or ports). Maybe we should consider refactoring the scan_socket method to not take &self (so we make it static) and instead have everything needed given to it as an argument (that would be tries and timeout). This would allow for other methods to use scan_socket for other strategies or so.

Yet, I can see how the easier change may be more appropriate here. I am fine with both versions, but I personally prefer the current approach.

[bernardoamc]

@niklasmohrin I'm fine with the current approach too, and when the need to further specialize this logic comes we can extract this logic to a self contained struct. Something like a "SocketStrategy" of sorts.

Most of my comments were around style, but @niklasmohrin and @bergabman did a great job reviewing the PR already. As soon as they are adjusted we can ship this great addition 🎉

[eiffel-fl]

I added all the reviews but stick with the implementation where tries takes place inside scan_socket.
Let me know if there is a problem or I I can improve something!

[bernardoamc]

Following my last comment , now we can use tries normally, so this would become let tries = self.tries;

[niklasmohrin]

Nice! I think it is almost perfect now. Additionally to these suggestions, the suggested refactoring on line 128 is not in yet.

Note: I think you should be able to close older suggestions with a button saying resolve conversation to make the page here a bit shorter 😄

[eiffel-fl]

Normally I added everything!
The button resolve conversation was in front of me and I did not see it 🤣🤣🤣!

[bergabman]

I tested it and made 2 small comments so the output gets improved a bit. Besides this I think we discussed the important parts and I'm happy with the final code. Thanks again for this great PR!

[eiffel-fl]

@bergabman You are welcome! And thanks also the others for your reviews!
I added them and push the new code.

[niklasmohrin]

Hey, I think I found a neat little adjustment: Since NonZeroU8 implements FromStr, you can actually require the flag to be non-zero all the way from the start. That means the user gets an explicit error instead of a silent adjustment:

$ target/debug/rustscan --tries 0 192.168.178.97 --no-nmap
error: Invalid value for '--tries <tries>': number would be zero for non-zero type

All I did was change u8 to NonZeroU8 in Config, Scanner etc. This also gets rid of the conversion in Scanner::new.

[bergabman]

Thanks again for the PR. Great code!

[bee-san]

@all-contributors please add @eiffel-fl for code
@all-contributors please add @niklasmohrin for code

[allcontributors]

@bee-san

I could not determine your intention.

Basic usage: @all-contributors please add @Someone for code, doc and infra

For other usages see the documentation

[bee-san]

@all-contributors please add @eiffel-fl for code

[allcontributors]

@bee-san

I've put up a pull request to add @eiffel-fl! 🎉

[bee-san]

@all-contributors please add @niklasmohrin for code

[allcontributors]

@bee-san

I've put up a pull request to add @niklasmohrin! 🎉

[niklasmohrin]

Uhhhm, @bee-san I actually only reviewed this PR 😄 I opened #277 yesterday, so I guess I am going to make up for the "code-contributor" title soon 😅

[bee-san]

Uhhhm, @bee-san I actually only reviewed this PR smile I opened #277 yesterday, so I guess I am going to make up for the "code-contributor" title soon sweat_smile

ahhh my bad, I saw your name in the reviews a lot so I thought I'd give it to ya 😆

[eiffel-fl]

Thank you for the merge!
Good language, good project and good community 😄!