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: Fix upload part copy range able to get almost any string #32487

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

ofriedma
Copy link
Contributor

@ofriedma ofriedma commented Jan 5, 2020

Fix upload part copy range able to get almost any string

This PR intends to add more checking on HTTP_X_AMZ_COPY_SOURCE_RANGE header

Signed-off-by: Or Friedmann ofriedma@redhat.com
Fixes: https://tracker.ceph.com/issues/43461

Fix upload part copy range able to get almost any string

This PR intends to add more checking on HTTP_X_AMZ_COPY_SOURCE_RANGE header

Signed-off-by: Or Friedmann <ofriedma@redhat.com>
Copy link
Contributor

@dang dang left a comment

Choose a reason for hiding this comment

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

Good changes, generally. One nit.

@@ -1785,8 +1786,20 @@ int RGWPutObj_ObjStore_S3::get_params()
}
string first = range.substr(0, pos);
string last = range.substr(pos + 1);
if (first.find_first_not_of("0123456789") != std::string::npos || last.find_first_not_of("0123456789") != std::string::npos)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use strict_strtoll() instead of manually checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dang
it would look like this to use strict_strtoll():
string *err_fst = new string();
string *err_lst = new string();
copy_source_range_fst = strict_strtoll(first.c_str(), 10, efst);
copy_source_range_lst = strict_strtoll(last.c_str(), 10, elst);
if (copy_source_range_fst > copy_source_range_lst || !efst->empty() || !elst->empty() || copy_source_range_fst < 0 || copy_source_range_lst < 0)
{
ret = -ERANGE;
ldpp_dout(this, 5) << "x-amz-copy-source-range bad format first number bigger than second" << dendl;
return ret;
}
}
I think it is adding more complex because we need to check for no error in the error string and check if we have negative numbers which we do not support with copy part.

what do you think to keeping it as it is now?

@theanalyst
Copy link
Member

also check GetObj::parse_range()

@dang dang added the needs-qa label Jan 16, 2020
@cbodley cbodley merged commit ce525d8 into ceph:master Jan 27, 2020
@cbodley
Copy link
Contributor

cbodley commented Jan 27, 2020

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

Successfully merging this pull request may close these issues.

4 participants