search

openbmc / bmcweb

A do everything Redfish, KVM, GUI, and DBus webserver for OpenBMC
Apache License 2.0
156 stars 131 forks source link

Crash with bad URL #144

Closed joseph-reynolds closed 4 years ago

joseph-reynolds commented 4 years ago

BMCWeb crashes when given a URL with a syntax error. For example, curl https://$bmc/'redfish\['. I get the response, curl: (52) Empty reply from server and logs from the command journalctl -u bmcweb shows (excerpts):

[REDACTED] bmcweb[225]: terminate called after throwing an instance of 'boost::wrapexcept<boost::urls::invalid_part>'
[REDACTED] bmcweb[225]:   what():  bad url argument
[REDACTED] systemd[1]: bmcweb.service: Main process exited, code=dumped, status=6/ABRT
[REDACTED] systemd[1]: bmcweb.service: Failed with result 'core-dump'.

This happens in recent versions only, like beginning July/August 2020. I believe a number of other syntax errors will also hit this problem. Note that systemd restarts bmcweb after it crashes.

I believe the problem was introduced here: https://github.ibm.com/openbmc/bmcweb/commit/5a7e877e5fd7da96022d3959fbfec84bfa3d0f7f#diff-a14a24ada9556db683fbdd6a15d3d084 Specifically, line 731 (req->urlView = boost::urls::url_view(req->target());) constructs an url_view which throws boost::urls::invalid_part. See https://github.com/CPPAlliance/url/blob/master/include/boost/url/url_view.hpp

A fix could be to catch the exception and issue HTTP status 400 (bad request). (I do not intend to submit a fix, but will help test it.) Please take a look. @feistjj

edtanous commented 4 years ago

A fix for this is already in review. https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/35707

Can you run your tests with it to verify it fixes this problem before its merged?

joseph-reynolds commented 4 years ago

Nope. Sorry, it already merged. I commented on it anyway. :-) I understand George plans to test it. I would like to see HTTP status 400 (bad request) for this, if possible.

edtanous commented 4 years ago

"I would like to see HTTP status 400 (bad request) for this, if possible." That's one possibility, on the other hand, they've broken the HTTP specification for urls, so bmcweb is fully within its rights to simply close the connection entirely.

I believe today it will return 404.

joseph-reynolds commented 4 years ago

Addressed by https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/35707 and https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/35744 Closing.