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

rgw: make init env methods return an error #20488

Merged
merged 1 commit into from Feb 23, 2018

Conversation

theanalyst
Copy link
Member

Since web frontends may signal an error when requests are malformed or so, let
us double check this and raise errors early. The current user of this is
civetweb frontend; which can potentially return null from parse_http_headers
when a HTTP header without a ":" is supplied at which point headers.value is
null which can lead to undefined behaviour later in RGW.

Fixes: http://tracker.ceph.com/issues/23039
Signed-off-by: Abhishek Lekshmanan abhishek@suse.com

Copy link
Member

@yehudasa yehudasa left a comment

Choose a reason for hiding this comment

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

@theanalyst see my comments


client_io->init(g_ceph_context);
int cio_error = 0;
cio_error = client_io->init(g_ceph_context);
Copy link
Member

Choose a reason for hiding this comment

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

@theanalyst can squash these two lines into one

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

init_env(cct);
[[nodiscard]] int BasicClient::init(CephContext *cct) {
int init_error=0;
init_error = init_env(cct);
Copy link
Member

Choose a reason for hiding this comment

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

can squash these two lines

Copy link
Member

Choose a reason for hiding this comment

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

also, maybe the [[nodiscard]] should be at the class definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

in this case introducing [[nodiscard]] at class definition generates a warning as RGWLibIO which derives from the basicClient will not respect the nodiscard flag


if (header->name == nullptr || header->value==nullptr) {
lderr(cct) << "client supplied malformatted headers" << dendl;
init_error=-EINVAL;
Copy link
Member

Choose a reason for hiding this comment

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

why do you set init_error here, can just return -EINVAL, right?

@@ -67,17 +67,25 @@ size_t RGWCivetWeb::complete_request()
return 0;
}

void RGWCivetWeb::init_env(CephContext *cct)
[[nodiscard]] int RGWCivetWeb::init_env(CephContext *cct)
Copy link
Member

Choose a reason for hiding this comment

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

@theanalyst shouldn't you have the [[nodiscard]] in the class definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

@theanalyst
Copy link
Member Author

changelog:

  • drop init_error variable in rgw_civetweb
  • raise -EINVAL if mg_request info isn't populated in rgw_civetweb.cc
    this is a changed behaviour from earlier where we return and fail at a much later stage in rgw process

@@ -20,7 +20,7 @@ ClientIO::ClientIO(tcp::socket& socket,

ClientIO::~ClientIO() = default;

void ClientIO::init_env(CephContext *cct)
int ClientIO::init_env(CephContext *cct)
Copy link
Contributor

@chardan chardan Feb 20, 2018

Choose a reason for hiding this comment

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

Consider the nodiscard attribute if this is intended for error detection.
(I personally think that you should throw if this is a critical failure, but I see that view is not universally shared.)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this particular code block doesn't return an error right now, thought I don't mind adding

@@ -119,8 +119,7 @@ int process_request(RGWRados* const store,
int* http_ret)
{
int ret = 0;

client_io->init(g_ceph_context);
int cio_error = client_io->init(g_ceph_context);
Copy link
Contributor

Choose a reason for hiding this comment

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

having 3 different variables (ret, cio_error, init_error) for error handling in this function is a bit much. can you use ret here, and add a new abort_early() check as soon as the req_state is initialized?

that way, we won't call rest->get_handler() unnecessarily, and the error handling for that call doesn't need to juggle the different return codes, ie. init_error ? init_error : cio_error

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried going that path before, If we return after just initializing rstate and return we assert later as s->cio is unset and abort_early uses that when dumping status.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me try setting cio before calling abort

@theanalyst theanalyst force-pushed the rgw/frontend-init branch 2 times, most recently from de67170 to 329db13 Compare February 20, 2018 15:15
Since web frontends may signal an error when requests are malformed or so, let
us double check this and raise errors early. The current user of this is
civetweb frontend; which can potentially return null from `parse_http_headers`
when a HTTP header without a ":" is supplied at which point headers.value is
null which can lead to undefined behaviour later in RGW.

Fixes: http://tracker.ceph.com/issues/23039
Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
@@ -138,13 +136,19 @@ int process_request(RGWRados* const store,
RGWObjectCtx rados_ctx(store, s);
s->obj_ctx = &rados_ctx;

if (ret < 0) {
s->cio = client_io;
Copy link
Member Author

Choose a reason for hiding this comment

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

@cbodley updated, had to set client_io here in order to send the response

@theanalyst
Copy link
Member Author

@yehudasa @cbodley do these changes look okay?

@cbodley
Copy link
Contributor

cbodley commented Feb 23, 2018

was unable to reproduce the failures in manual testing 👍

@cbodley cbodley merged commit 2b84500 into ceph:master Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants