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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove a panic in normalize middleware #1762

Merged
merged 2 commits into from
Dec 1, 2020
Merged

Conversation

maciejhirsz
Copy link
Contributor

@maciejhirsz maciejhirsz commented Oct 29, 2020

PR Type

Big Fix

PR Checklist

  • Tests for the changes have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt

Overview

Fixes a panic that occurred occasionally when using path normalization middleware. If the original path was empty due to a malformed request, parts.path_and_query would be None. Actual panic logged on a live system 馃檭:

Oct 29 14:46:09 polkadot-telemetry-0 a1e8ee04ee4a[678]: thread 'actix-rt:worker:2' panicked at 'called `Option::unwrap()` on a `None` value', /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/actix-web-3.1.0/src/middleware/normalize.rs:140:22

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me 馃憤

CHANGES.md Outdated
Comment on lines 20 to 22
### Fixed
* Removed an occasional `unwrap` on `None` panic in `NormalizePathNormalization`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to unreleased section

@robjtede
Copy link
Member

robjtede commented Nov 2, 2020

is it possible to contrive a test for this? whats the exact condition?

@maciejhirsz
Copy link
Contributor Author

@robjtede unfortunately my server logs couldn't give me a definitive request URL that triggered it, so I weren't able to reproduce it manually, but it was happening frequently enough to warrant me running a patched version of actix-web. From what I gathered by reading code, this fails when the path is missing either the scheme (no http: etc) or is missing a path (/ is fine).

@JohnTitor
Copy link
Member

I prefer just to merge this rather than to wait for coming up with a suitable test as this is a general improvement.

@robjtede Thoughts?

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #1762 (09ae9c9) into master (32d59ca) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1762   +/-   ##
=======================================
  Coverage   53.80%   53.80%           
=======================================
  Files         129      129           
  Lines       12266    12266           
=======================================
  Hits         6600     6600           
  Misses       5666     5666           
Impacted Files Coverage 螖
src/middleware/normalize.rs 99.30% <100.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 32d59ca...09ae9c9. Read the comment docs.

@robjtede
Copy link
Member

robjtede commented Dec 1, 2020

Yep it's a sensible change regardless.

@JohnTitor JohnTitor merged commit 7981e00 into actix:master Dec 1, 2020
@JohnTitor
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web B-semver-patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants