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

Prevent parsing brackets from causing panic #108

Merged
merged 1 commit into from May 11, 2015

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented May 11, 2015

The code:

Url::parse("http://[]")

The panic:

thread '<main>' panicked at 'index out of bounds: the len is 0 but the index is 0',
/Users/coreyf/.cargo/registry/src/github.com-1ecc6299db9ec823/url-0.2.34/src/host.rs:104

in addition to:

The code:

Url::parse("http://[:]")

The panic:

/Users/coreyf/.cargo/registry/src/github.com-1ecc6299db9ec823/url-0.2.34/src/host.rs:104
thread 'tests::url_parsing' panicked at 'index out of bounds: the len is
1 but the index is 1', src/host.rs:110

Was found using https://github.com/kmcallister/afl.rs 👍

@frewsxcv frewsxcv force-pushed the frewsxcv:prevent-panic branch 2 times, most recently from dce2818 to b0e900f May 11, 2015
frewsxcv added a commit to frewsxcv/web-platform-tests that referenced this pull request May 11, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented May 11, 2015

Dont merge this yet, theres another bug in here

@SimonSapin
Copy link
Member

SimonSapin commented May 11, 2015

Nice! But I think a similar issue exists for Url::parse("http://[:]"). Could you change if len == 0 to if len < 2? Reading the spec, I think any input of length 1 is also invalid.

@frewsxcv
Copy link
Member Author

frewsxcv commented May 11, 2015

@SimonSapin Yeah, I was just about to add that in :)

The code:

```
Url::parse("http://[]")
```

The panic:

```
thread '<main>' panicked at 'index out of bounds: the len is 0 but the index is 0',
/Users/coreyf/.cargo/registry/src/github.com-1ecc6299db9ec823/url-0.2.34/src/host.rs:104
```

in addition to:

The code:

```
Url::parse("http://[:]")
```

The panic:

```
/Users/coreyf/.cargo/registry/src/github.com-1ecc6299db9ec823/url-0.2.34/src/host.rs:104
thread 'tests::url_parsing' panicked at 'index out of bounds: the len is
1 but the index is 1', src/host.rs:110
```

Was found using https://github.com/kmcallister/afl.rs 👍
@frewsxcv frewsxcv force-pushed the frewsxcv:prevent-panic branch from b0e900f to 1f08064 May 11, 2015
@frewsxcv frewsxcv changed the title Prevent `http://[]` from causing panic @frewsxcv Prevent parsing brackets from causing panic May 11, 2015
@frewsxcv frewsxcv changed the title @frewsxcv Prevent parsing brackets from causing panic Prevent parsing brackets from causing panic May 11, 2015
frewsxcv added a commit to frewsxcv/web-platform-tests that referenced this pull request May 11, 2015
frewsxcv added a commit to frewsxcv/web-platform-tests that referenced this pull request May 11, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented May 11, 2015

Should be reviewable; tests pass

SimonSapin added a commit that referenced this pull request May 11, 2015
Prevent parsing brackets from causing panic
@SimonSapin SimonSapin merged commit d8a7e90 into servo:master May 11, 2015
2 checks passed
2 checks passed
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SimonSapin
Copy link
Member

SimonSapin commented May 11, 2015

Thanks!

@frewsxcv frewsxcv deleted the frewsxcv:prevent-panic branch May 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.