-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix capacity overflow caused by large range of ports #11210
Conversation
I tried to test this but I'm not really sure it's possible. I tried using numbers that are too high and it just picks a lower one, which was surprising. ❯ port 65536 999999999999999999
50275 Unrelated to this PR but it seems to me like we should use a range here instead of two parameters like |
This did crash on my Linux box. And I think the following code has performance issues when the range is very large, there is no need to collect so many addresses, because we just pick one of them. nushell/crates/nu-command/src/network/port.rs Lines 104 to 112 in f36c055
|
|
Right, but I was trying to get something higher than that to try and produce your new error message.
That's freaky - you didn't have any ports available at all? |
release build vs debug build? |
Both release and debug builds crashed on my machine. :( |
I wondered why mine wasn't crashing and it was because I was testing your PR. LOL. Let's move forward with this. Thanks! |
.collect(); | ||
TcpListener::bind(addrs.as_slice())? | ||
match (start_port..=end_port) | ||
.map(|port| SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, port as u16))) |
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.
Mhh the truncating as u16
coercion is still there, so this seems to fix another part of error handling but doesn't prevent the overflow itself? Maybe worth migrating to try_into
or explicit checking beforehand that it is in the valid range of a port.
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 PR fixes Vec<SocketAddr>
overflow. Casting port
to u16
is another issue. I think we could check port
explicitly.
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.
I created a pr #11214.
# Description This PR adds checks for ports. This fixes unexpected output similar to the one in the comment #11210 (comment). * before ```console /data/source/nushell> port 65536 99999 41233 ``` * after ```console /data/source/nushell> port 65536 99999 Error: nu::shell::cant_convert × Can't convert to u16. ╭─[entry #1:1:1] 1 │ port 65536 99999 · ──┬── · ╰── can't convert usize to u16 ╰──── help: out of range integral type conversion attempted (min: 0, max: 65535) ``` # User-Facing Changes N/A # Tests + Formatting * [x] add `port_out_of_range` test # After Submitting N/A
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> Try to fix capacity overflow caused by large range of ports. ``` $ port 1024 999999999999999999 12/02/23 20:03:14 PM thread 'main' panicked at 'capacity overflow', library/alloc/src/raw_vec.rs:524:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
# Description This PR adds checks for ports. This fixes unexpected output similar to the one in the comment nushell#11210 (comment). * before ```console /data/source/nushell> port 65536 99999 41233 ``` * after ```console /data/source/nushell> port 65536 99999 Error: nu::shell::cant_convert × Can't convert to u16. ╭─[entry #1:1:1] 1 │ port 65536 99999 · ──┬── · ╰── can't convert usize to u16 ╰──── help: out of range integral type conversion attempted (min: 0, max: 65535) ``` # User-Facing Changes N/A # Tests + Formatting * [x] add `port_out_of_range` test # After Submitting N/A
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> Try to fix capacity overflow caused by large range of ports. ``` $ port 1024 999999999999999999 12/02/23 20:03:14 PM thread 'main' panicked at 'capacity overflow', library/alloc/src/raw_vec.rs:524:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
# Description This PR adds checks for ports. This fixes unexpected output similar to the one in the comment nushell#11210 (comment). * before ```console /data/source/nushell> port 65536 99999 41233 ``` * after ```console /data/source/nushell> port 65536 99999 Error: nu::shell::cant_convert × Can't convert to u16. ╭─[entry nushell#1:1:1] 1 │ port 65536 99999 · ──┬── · ╰── can't convert usize to u16 ╰──── help: out of range integral type conversion attempted (min: 0, max: 65535) ``` # User-Facing Changes N/A # Tests + Formatting * [x] add `port_out_of_range` test # After Submitting N/A
Description
Try to fix capacity overflow caused by large range of ports.
User-Facing Changes
Tests + Formatting
After Submitting