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

Ssh-rs panic on a thread when using in multi threading #77

Closed
Paulo-21 opened this issue Oct 12, 2023 · 4 comments · Fixed by #78
Closed

Ssh-rs panic on a thread when using in multi threading #77

Paulo-21 opened this issue Oct 12, 2023 · 4 comments · Fixed by #78
Assignees
Labels
bug Something isn't working

Comments

@Paulo-21
Copy link

Hello,
I had an issue with your library.
One thread panic when i use more threads than i have physical cores.
When i use Available parallelism from the rust standard library, i got 12 cores.
I precise that my cpu have 8 cores without hyper threading and 2 physical cores with Hyper Threading.
So 10 physical core without hyper threading.

When i set the number of threads to 10, no crash happen but when i increase the number of threads to 11 or 12 then it crash.

i m just running n threads and running ssh inside of each one.

thread '<unnamed>' panicked at C:\Users\Username\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ssh-rs-0.4.1\src\config\version.rs:36:17:
assertion `left == right` failed
  left: [69, 120, 99, 101]
 right: [83, 83, 72, 45]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@HsuJv
Copy link
Collaborator

HsuJv commented Oct 12, 2023

Hi,

May I have a copy of your PoC code?

It looks strange cuz this assert is only to test the first 4 bytes of the ssh server replies when we connected.
According to the RFC, it must be "SSH-xxxxxx", which in ASCII is [83, 83, 72, 45, xxxxx]

A fatal here is that we shall throw an error rather than panic. But if you're connecting a true ssh server, it is worth investigating more about how could it happen.

BRs.

@HsuJv HsuJv added the bug Something isn't working label Oct 12, 2023
@HsuJv HsuJv self-assigned this Oct 12, 2023
@Paulo-21
Copy link
Author

Hi, thank you for your help.
Ssh Server : OpenSSH_8.9p1

PoC

use ssh;
use std::thread::{self, JoinHandle};
use std::thread::available_parallelism;
fn main() {
    //let default_parallelism_approx = available_parallelism().unwrap().get();
    let default_parallelism_approx = 11;
    let mut threads = Vec::<JoinHandle<()>>::new();
    for i in 0..default_parallelism_approx {
        let mut start = i;
        let e = thread::spawn(move || {
            let session_name = "Admin";
	    let password = "12345678";
            while start < 10000 {
                let connection = ssh::create_session()
                .username(&session_name)
                .password(&password)
                .connect("wwww.website.com:22");

        }
        });
        threads.push(e);
    }
    for _u in 0..default_parallelism_approx {
        let r = threads.pop().unwrap();
        let _ = r.join();
    }
}

@HsuJv
Copy link
Collaborator

HsuJv commented Oct 13, 2023

Hi @Paulo-21

With #78 it now returns an error SshError::VersionDismatchError rather than panic.

It's actually some error message which indicates that the server is not able to handle more clients.

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: VersionDismatchError { our: "SSH-2.0-SSH_RS-0.4.1", their: "Exceeded MaxStartups" }', exec/src/main.rs:38:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: VersionDismatchError { our: "SSH-2.0-SSH_RS-0.4.1", their: "Exceeded MaxStartups" }', exec/src/main.rs:38:18
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: VersionDismatchError { our: "SSH-2.0-SSH_RS-0.4.1", their: "Exceeded MaxStartups" }', exec/src/main.rs:38:18
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: VersionDismatchError { our: "SSH-2.0-SSH_RS-0.4.1", their: "Exceeded MaxStartups" }', exec/src/main.rs:38:18

I'll later have 0.4.2 released to include the fix.

Thank you for reporting!

BRs.

@Paulo-21
Copy link
Author

Hi @HsuJv
Thank you for your reactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants