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

Transmute in headers does not look safe #12

Closed
reem opened this issue Sep 6, 2014 · 3 comments · Fixed by #14
Closed

Transmute in headers does not look safe #12

reem opened this issue Sep 6, 2014 · 3 comments · Fixed by #14

Comments

@reem
Copy link
Contributor

reem commented Sep 6, 2014

I am not convinced that this is safe: https://github.com/seanmonstar/hyper/blob/master/src/header.rs#L96

I think I could trivially easily cause a segfault with this, by moving the headers out of a Request and sending them across a channel so they live arbitrarily long.

@reem
Copy link
Contributor Author

reem commented Sep 6, 2014

On further inspection, I'm actually surprised this works at all... Shouldn't the name defined here (https://github.com/seanmonstar/hyper/blob/master/src/header.rs#L93) be dropped at the end of the function, leaving you with a dangling pointer to it?

@seanmonstar
Copy link
Member

Hrm, you're right, that should be a dangling pointer... does it manage to work because HashMap stores the hash u64, instead of the actual &str, and so when checking later, it looks up by u64?

@reem
Copy link
Contributor Author

reem commented Sep 7, 2014

It has to store the actual string to resolve collisions, but it will only access the string if there is a collision. Given that most of the examples insert a limited number of headers, it's likely that there just aren't any collisions to resolve in them.Thanks,
Jonathan Reem

On Sat, Sep 6, 2014 at 11:15 PM, Sean McArthur notifications@github.com
wrote:

Hrm, you're right, that should be a dangling pointer... does it manage to work because HashMap stores the hash u64, instead of the actual &str, and so when checking later, it looks up by u64?

Reply to this email directly or view it on GitHub:
#12 (comment)

reem added a commit to reem/hyper that referenced this issue Sep 7, 2014
This fixes an unsafe transmute that could cause use-after-frees
on collision resolution in the headers hash. The new scheme allocates
Strings on insertion, but prevents most allocations on lookup by using
the `equiv` methods for querying by &str.

Fixes hyperium#12
reem added a commit to reem/hyper that referenced this issue Sep 8, 2014
This fixes an unsafe transmute that could cause use-after-frees
on collision resolution in the headers hash and avoids allocating
for cases where it is not necessary.

Fixes hyperium#12
reem added a commit to reem/hyper that referenced this issue Sep 8, 2014
This fixes an unsafe transmute that could cause use-after-frees
on collision resolution in the headers hash and avoids allocating
for cases where it is not necessary.

Fixes hyperium#12
reem added a commit to reem/hyper that referenced this issue Sep 8, 2014
This fixes an unsafe transmute that could cause use-after-frees
on collision resolution in the headers hash and avoids allocating
for cases where it is not necessary.

Fixes hyperium#12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants