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.

Sign up for GitHub

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

Jump to bottom

rgw: Do not forward functor objects in rgw_iam_policy.h #51676

Merged
merged 1 commit into from Jun 15, 2023
Merged

rgw: Do not forward functor objects in rgw_iam_policy.h #51676

merged 1 commit into from Jun 15, 2023

Conversation

vedanshbhartia
Copy link
Contributor

@vedanshbhartia vedanshbhartia commented May 22, 2023 •
edited

rgw_iam_policy.h implements the andible, orrible, and shortible methods which called forward on functor objects called in a loop. This does not have any effect currently since the callers do not pass stateful functors. However, in case a stateful functor that is also an rvalue is used, the forward would cause it to lose its state.

Fixes coverity defects 1510264, 1511592, 1511865

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@vedanshbhartia vedanshbhartia requested a review from a team as a code owner May 22, 2023 17:31
@github-actions github-actions bot added the rgw label May 22, 2023
adamemerson
adamemerson approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@adamemerson adamemerson left a comment

Choose a reason for hiding this comment

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

Good catch, thank you.

@mattbenjamin mattbenjamin requested review from adamemerson and a team May 22, 2023 17:33
yuvalif
yuvalif reviewed May 22, 2023
View reviewed changes
src/rgw/rgw_iam_policy.h Outdated
if (std::forward<F>(f)(itr->second, d)) {
return true;
if (f(itr->second, d)) {
return true`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return true`;
return true;

yuvalif
yuvalif reviewed May 22, 2023
View reviewed changes
src/rgw/rgw_iam_policy.h Outdated
if (!xd) {
continue;
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
continue;
continue;

yuvalif
yuvalif reviewed May 22, 2023
View reviewed changes
src/rgw/rgw_iam_policy.h Outdated
if (std::forward<F>(f)(*xc, *xd)) {
return true;
if (f(*xc, *xd)) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return true;
return true;

@vedanshbhartia
rgw: Do not forward functor objects in rgw_iam_policy.h
ec5d0bd 
rgw_iam_policy.h implements the andible, orrible, and shortible methods
which called forward on functor objects called in a loop. This does not
have any effect currently since the callers do not pass stateful
functors. However, in case a stateful functor that is also an rvalue is
used, the forward would cause it to lose its state.

Signed-off-by: Vedansh Bhartia <vedanshbhartia@gmail.com>
@yuvalif yuvalif self-requested a review June 8, 2023 19:11
@yuvalif
Copy link
Contributor

yuvalif commented Jun 12, 2023

teuthology results: http://pulpito.ceph.com/yuvalif-2023-06-11_16:58:37-rgw-wip-yuval-test1-distro-default-smithi/
some failures similar to baseline: multisite, valgrind, keystone, "Cannot create secret"
but also these failures:

s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_upload - b...
s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_invalid_chunks_1
s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_invalid_chunks_2
s3tests_boto3/functional/test_s3.py::test_sse_kms_transfer_1b - botoco...
s3tests_boto3/functional/test_s3.py::test_sse_kms_transfer_1kb - botoc...

@yuvalif
Copy link
Contributor

yuvalif commented Jun 12, 2023

teuthology results: http://pulpito.ceph.com/yuvalif-2023-06-11_16:58:37-rgw-wip-yuval-test1-distro-default-smithi/ some failures similar to baseline: multisite, valgrind, keystone, "Cannot create secret" but also these failures:

s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_upload - b...
s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_invalid_chunks_1
s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_invalid_chunks_2
s3tests_boto3/functional/test_s3.py::test_sse_kms_transfer_1b - botoco...
s3tests_boto3/functional/test_s3.py::test_sse_kms_transfer_1kb - botoc...

@alimaredia can you please have a look at the teuthology results?

@cbodley
Copy link
Contributor

cbodley commented Jun 12, 2023 

s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_upload - b...
s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_invalid_chunks_1
s3tests_boto3/functional/test_s3.py::test_sse_kms_multipart_invalid_chunks_2
s3tests_boto3/functional/test_s3.py::test_sse_kms_transfer_1b - botoco...
s3tests_boto3/functional/test_s3.py::test_sse_kms_transfer_1kb - botoc...

those are tracked in https://tracker.ceph.com/issues/58751. they only fail in rgw/crypt with kmip

@alimaredia
Copy link
Contributor

@cbodley @yuvalif same with this crash in a reshard workunit here. Is this to be expected: http://qa-proxy.ceph.com/teuthology/yuvalif-2023-06-11_16:58:37-rgw-wip-yuval-test1-distro-default-smithi/7301224/teuthology.log

@alimaredia alimaredia mentioned this pull request Jun 12, 2023
Merged
14 tasks
@yuvalif
Copy link
Contributor

yuvalif commented Jun 13, 2023

@cbodley @yuvalif same with this crash in a reshard workunit here. Is this to be expected: http://qa-proxy.ceph.com/teuthology/yuvalif-2023-06-11_16:58:37-rgw-wip-yuval-test1-distro-default-smithi/7301224/teuthology.log

it looks like an intentioanl abort. from the code and the backtrace it looks like radosgw-admin reshard command was called with "--inject-abort-at" flag.

@yuvalif yuvalif merged commit 7142708 into ceph:main Jun 15, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuvalif yuvalif yuvalif approved these changes

@adamemerson adamemerson Awaiting requested review from adamemerson

Assignees
No one assigned
Labels
GSoC / Outreachy needs-qa rgw wip-yuval-testing
Projects
None yet
Milestone
No milestone
5 participants
@vedanshbhartia @yuvalif @cbodley @alimaredia @adamemerson