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

Unexpected panic when using header::OccupiedEntry::remove_entry_mult #446

Closed
l3ku opened this issue Nov 24, 2020 · 4 comments
Closed

Unexpected panic when using header::OccupiedEntry::remove_entry_mult #446

l3ku opened this issue Nov 24, 2020 · 4 comments
Labels
A-headers Area: HTTP headers S-bug Severity: bug. Something is wrong!

Comments

@l3ku
Copy link

l3ku commented Nov 24, 2020

I am trying to remove all possible values for a SET_COOKIE header from a HeaderMap, and extract them into a separate variable called cookies. My current code is the following:

let response: Response<Body> = Response::builder()
    .header("set-cookie", "cookie1=a")
    .header("set-cookie", "cookie2=b")
    .body(Body::from(()))
    .expect("failed to create response");

let (parts, body) = response.into_parts();
let mut headers = parts.headers;

...

let cookies: Vec<HeaderValue> = match headers.entry(SET_COOKIE) {
    Entry::Occupied(e) => e.remove_entry_mult().1.collect(),
    Entry::Vacant(_) => vec![],
};

When I run this, I get the following panic:

thread 'response::tests::serialize_cookies' panicked at 'index out of bounds: the len is 0 but the index is 0', /Users/leotoikka/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/http-0.2.0/src/header/map.rs:1606:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Relevant parts of the backtrace when ran with RUST_BACKTRACE=1:

   9:        0x10b656034 - <http::header::map::RawLinks<T> as core::ops::index::IndexMut<usize>>::index_mut::haae19e4b2ee84e7b
                               at /Users/leotoikka/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/http-0.2.0/src/header/map.rs:3102
  10:        0x10b654e06 - http::header::map::remove_extra_value::hf7cd983444b37b83
                               at /Users/leotoikka/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/http-0.2.0/src/header/map.rs:1606
  11:        0x10b5c3414 - <http::header::map::ValueDrain<T> as core::iter::traits::iterator::Iterator>::next::h734d41d3af084598
                               at /Users/leotoikka/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/http-0.2.0/src/header/map.rs:3042
  12:        0x10b54858d - <http::header::map::ValueDrain<T> as core::ops::drop::Drop>::drop::h081f86fb65b64b3d
                               at /Users/leotoikka/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/http-0.2.0/src/header/map.rs:3072
  13:        0x10b5478c5 - core::ptr::drop_in_place::he2a9208b609548b1

Am I doing something wrong or is this a bug?

Thanks!
@l3ku l3ku changed the title Unexpected panic when using header::OccupiedEntry::remove_entry or remove_entry_mult Unexpected panic when using header::OccupiedEntry::remove_entry_mult Nov 24, 2020
@l3ku l3ku mentioned this issue Nov 24, 2020
Merged
2 tasks
@seanmonstar
Copy link
Member

It does look like a bug in http. However, I don't think the panic is inside remote_entry_mult(), but rather happening during (after?) the call to collect(), since the backtrace shows it's happening as part of the Iterator::next. I'd have to look more to try to find why exactly, unless you wanted to try to follow the code and find the bug.

@seanmonstar seanmonstar added A-headers Area: HTTP headers S-bug Severity: bug. Something is wrong! labels Nov 24, 2020
dekellum added a commit to dekellum/http that referenced this issue Nov 24, 2020
@dekellum
add test case for OccupiedEntry::remove_entry_mult
19e82f4 
This is just a self contained test case, currently reproducing the
panic of hyperium#446.
@dekellum
Copy link
Contributor

I had a related test case in flight, so I minimally reproduced this in 19e82f4. Note that it only panics with 2 or more header values for the same name; it doesn't panic on 0 or 1 header value.

@l3ku
Copy link
Author

l3ku commented Nov 25, 2020

I can try to look into it, but I'm not very familiar with the implementation of HeaderMap so it might take me some time to find the root cause for the panic.

@dekellum dekellum mentioned this issue Dec 1, 2020
Merged
@dekellum
Copy link
Contributor

dekellum commented Dec 2, 2020

I believe #449 is now a reasonable fix. Note that tests added there show the bug extends to OccupiedEntry::remove_entry() as well.

@dekellum dekellum mentioned this issue Dec 14, 2020
Merged
BenxiangGe pushed a commit to BenxiangGe/http that referenced this issue Jul 26, 2021
@dekellum
Fixes panic for remove_entry and remove_entry_mult (hyperium#449)
84e2253 
* add test case for OccupiedEntry::remove_entry_mult

This is just a self contained test case, currently reproducing the
panic of hyperium#446.

* expand test cases for remove_entry_mult

* add multiple remove_entry_mult call tests to show more issues

* test repeated HeaderMap::remove for comparison

* tests showing similar problem with remove_entry on extra values

* fix remove_entry by moving remove_found after remove_all_extra_values

* fix remove_entry_mult by eager collection of extras before remove_found

In order to remove extra values prior to remove_found, we must collect
them eagerly. Eager collection is based on:

    commit 8ffe094
    Author:     Sean McArthur <[email protected]>
    AuthorDate: Mon Nov 25 17:34:30 2019 -0800
    Commit:     Sean McArthur <[email protected]>
    CommitDate: Tue Nov 26 10:03:09 2019 -0800

	Make ValueDrain eagerly collect its extra values

...which was reverted in 6c2b789.

Closes hyperium#446
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-headers Area: HTTP headers S-bug Severity: bug. Something is wrong!
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@seanmonstar @dekellum @l3ku