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.

Sign up for GitHub

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

Jump to bottom

Panics when parsing a single hyphen #410

Closed
cite-reader opened this issue Feb 3, 2016 · 4 comments
Closed

Panics when parsing a single hyphen #410

cite-reader opened this issue Feb 3, 2016 · 4 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies

Comments

@cite-reader
Copy link
Contributor

Given the following minimal test case:

[package]
name = "repro"
version = "0.1.0"

[dependencies]
clap = "2.0.2"
extern crate clap;

fn main() {
    clap::App::new("repro").get_matches();
}

running the resulting executable as ./target/debug/repro - produces the error thread '<main>' panicked at 'index out of bounds: the len is 1 but the index is 1', /home/cite-reader/.multirust/toolchains/stable/cargo/registry/src/jackfan.us.kg-88ac128001ac3a9a/clap-2.0.2/src/osstringext.rs:42.

Running with RUST_BACKTRACE=1 additionally produces this wall of text:

stack backtrace:
   1:     0x55d96739e470 - sys::backtrace::tracing::imp::write::haa19c02b4de52f3bG0t
   2:     0x55d9673a07a5 - panicking::log_panic::_<closure>::closure.41218
   3:     0x55d9673a0220 - panicking::log_panic::h527fe484e9de8fe1W7x
   4:     0x55d9673961a3 - sys_common::unwind::begin_unwind_inner::h51f64b1a34c60827fTs
   5:     0x55d967396538 - sys_common::unwind::begin_unwind_fmt::h0845853a1913f45blSs
   6:     0x55d96739dad1 - rust_begin_unwind
   7:     0x55d9673ccf2f - panicking::panic_fmt::h3967afc085fe8067LFK
   8:     0x55d9673cd0a2 - panicking::panic_bounds_check::h3ef0001c29fc27faREK
   9:     0x55d9673120ef - osstringext::_<impl>::starts_with::h26506bceadbe96d4Gkj
                        at /home/cite-reader/.multirust/toolchains/stable/cargo/registry/src/jackfan.us.kg-88ac128001ac3a9a/clap-2.0.2/src/osstringext.rs:42
  10:     0x55d967309837 - app::parser::_<impl>::get_matches_with::h2749516486128734984
                        at /home/cite-reader/.multirust/toolchains/stable/cargo/registry/src/jackfan.us.kg-88ac128001ac3a9a/clap-2.0.2/src/app/parser.rs:475
  11:     0x55d967307ed3 - app::_<impl>::get_matches_from_safe_borrow::h15298357069185562126
                        at /home/cite-reader/.multirust/toolchains/stable/cargo/registry/src/jackfan.us.kg-88ac128001ac3a9a/clap-2.0.2/src/app/mod.rs:788
  12:     0x55d967305b9f - app::_<impl>::get_matches_from::h16246934518760431591
                        at /home/cite-reader/.multirust/toolchains/stable/cargo/registry/src/jackfan.us.kg-88ac128001ac3a9a/clap-2.0.2/src/app/mod.rs:700
  13:     0x55d967305a9d - app::_<impl>::get_matches::h2e07d26000a6a67e1xf
                        at /home/cite-reader/.multirust/toolchains/stable/cargo/registry/src/jackfan.us.kg-88ac128001ac3a9a/clap-2.0.2/src/app/mod.rs:653
  14:     0x55d9672af440 - main::h3ed4f383bb959950faa
                        at src/main.rs:4
  15:     0x55d96739ffc4 - sys_common::unwind::try::try_fn::h11901883998771707766
  16:     0x55d96739d918 - __rust_try
  17:     0x55d96739fc66 - rt::lang_start::hc150f651dd2af18b44x
  18:     0x55d9672b2339 - main
  19:     0x7f67d556360f - __libc_start_main
  20:     0x55d9672af2e8 - _start
  21:                0x0 - <unknown>
@cite-reader
Copy link
Contributor Author

Is there a reason that starts_with method isn't just the following?

fn starts_with(&self, s: &[u8]) -> bool {
    self.as_bytes().starts_with(s)
}

@kbknapp
Copy link
Member

kbknapp commented Feb 3, 2016

Thanks for taking the time to file this! I see you pushed a commit to your fork, feel free to submit the PR!

@kbknapp kbknapp added C-bug Category: Updating dependencies P2: need to have A-parsing Area: Parser's logic and needs it changed somehow. labels Feb 3, 2016
@kbknapp
Copy link
Member

kbknapp commented Feb 3, 2016

@cite-reader Also, your commit message is excellent! This is one of many things I need to work harder on (read, "actually do").

@cite-reader cite-reader mentioned this issue Feb 3, 2016
Merged
homu added a commit that referenced this issue Feb 4, 2016
@homu
Auto merge of #411 - cite-reader:hyphen-doesnt-panic, r=kbknapp
1ede878 
fix: Stop lonely hyphens from causing panic

The method `starts_with` as implemented for the `OsStrExt2` trait on
`OsStr` assumed that the needle given is shorter than the haystack. When
this is not the case, the method panics due to an attempted
out-of-bounds access on the byte representation of `self`. Problematic
if, say, an end-user gives us `"-"` and the library tries to see if that
starts with `"--"`.

Fortunately, slices already implement a `starts_with` method, and we can
delegate to it.

This *does* create a semantics change: if both `self` and the needle
have length 0, this implementation will return `true`, but the old
implementation would return `false`. Based on the test suite still
passing, acknowledging the vacuous truth doesn't seem to cause any
problems.

Fixes #410
homu added a commit that referenced this issue Feb 4, 2016
@homu
Auto merge of #411 - cite-reader:hyphen-doesnt-panic, r=kbknapp
355a5fd 
fix: Stop lonely hyphens from causing panic

The method `starts_with` as implemented for the `OsStrExt2` trait on
`OsStr` assumed that the needle given is shorter than the haystack. When
this is not the case, the method panics due to an attempted
out-of-bounds access on the byte representation of `self`. Problematic
if, say, an end-user gives us `"-"` and the library tries to see if that
starts with `"--"`.

Fortunately, slices already implement a `starts_with` method, and we can
delegate to it.

This *does* create a semantics change: if both `self` and the needle
have length 0, this implementation will return `true`, but the old
implementation would return `false`. Based on the test suite still
passing, acknowledging the vacuous truth doesn't seem to cause any
problems.

Fixes #410
@homu homu closed this as completed in #411 Feb 4, 2016
@kbknapp
Copy link
Member

kbknapp commented Feb 4, 2016

once #413 merges I'll put out the new version on crates.io

That PR also adds tests so we shouldn't run into this issue again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@cite-reader @kbknapp