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: Add coverity uninitialized variable and initialize RGWBucketEntryMetadataObject #52254

Merged
merged 1 commit into from Oct 9, 2023

Conversation

vedanshbhartia
Copy link
Contributor

Contribution Guidelines

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

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@yuvalif
Copy link
Contributor

yuvalif commented Jul 3, 2023

@vedanshbhartia could you please squash the commits?

@yuvalif
Copy link
Contributor

yuvalif commented Jul 3, 2023

jenkins test api

@@ -1656,7 +1656,7 @@ class RGWBucketMetadataHandler : public RGWBucketMetadataHandlerBase {
class RGWMetadataHandlerPut_Bucket : public RGWMetadataHandlerPut_SObj
{
RGWBucketMetadataHandler *bhandler;
RGWBucketEntryMetadataObject *obj;
RGWBucketEntryMetadataObject *obj = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure this is the correct behavior?
maybe the right fix is to pass _obj to the ctor of RGWMetadataHandlerPut_SObj?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RGWMetadataHandlerPut_SObj doesn't have the context of obj though, it is defined in RGWMetadataHandlerPut_Bucket.

Copy link
Contributor

Choose a reason for hiding this comment

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

so, why do we pass it to the RGWMetadataHandlerPut_SObj constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

obj is ultimately used to initialize another RGWMetadataObject inside RGWMetadataHandler_GenericMetaBE::Put::Put

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. but this is still an issue.
we pass nullptr to RGWMetadataHandler_GenericMetaBE::Put::Put but store the object pointer to RGWMetadataHandlerPut_Bucket.
we should probably try to ask what is the right fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kalebskeithley this was done as part of 5ba9842
should we pass _obj to RGWMetadataHandlerPut_SObj ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vedanshbhartia could you please fix as discussed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -124,6 +124,7 @@ int RGWRESTConn::forward_iam_request(const DoutPrefixProvider *dpp, const RGWAcc
}
std::string service = "iam";
RGWRESTSimpleRequest req(cct, info.method, url, NULL, &params, api_name);
// coverity[uninit_use_in_call:SUPPRESS]
Copy link
Contributor

Choose a reason for hiding this comment

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

on which uninitialized variable was the complaint?

Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

req.api_name

@@ -143,6 +144,7 @@ int RGWRESTConn::put_obj_send_init(const rgw_obj& obj, const rgw_http_param_pair
}

RGWRESTStreamS3PutObj *wr = new RGWRESTStreamS3PutObj(cct, "PUT", url, NULL, &params, api_name, host_style);
// coverity[uninit_use_in_call:SUPPRESS]
Copy link
Contributor

Choose a reason for hiding this comment

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

on which uninitialized variable was the complaint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wr->api_name, which is an optional string. Coverity was complaining about an internal member of the optional string
Coverity issue ID 1521574

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. got it. so, this is a know coverity bug to treat std::optional as uninit

@@ -160,6 +162,7 @@ int RGWRESTConn::put_obj_async_init(const DoutPrefixProvider *dpp, const rgw_use
param_vec_t params;
populate_params(params, &uid, self_zone_group);
RGWRESTStreamS3PutObj *wr = new RGWRESTStreamS3PutObj(cct, "PUT", url, NULL, &params, api_name, host_style);
// coverity[uninit_use_in_call:SUPPRESS]
Copy link
Contributor

Choose a reason for hiding this comment

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

on which uninitialized variable was the complaint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wr->api_name, which is an optional string. Coverity was complaining about an internal member of the optional string
Coverity issue ID 1521567

@@ -290,6 +293,7 @@ int RGWRESTConn::get_obj(const DoutPrefixProvider *dpp, const rgw_obj& obj, cons
set_header(buf, extra_headers, "RANGE");
}

// coverity[uninit_use_in_call:SUPPRESS]
Copy link
Contributor

Choose a reason for hiding this comment

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

on which uninitialized variable was the complaint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

req->api_name, which is an optional string.
Coverity issue ID 1521575

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't see api_name in line 297

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used inside the call chain of send_prepare

@yuvalif
Copy link
Contributor

yuvalif commented Jul 23, 2023

jenkins test api

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

…ndlerPut_SObj correctly

Signed-off-by: Vedansh Bhartia <vedanshbhartia@gmail.com>
@yuvalif
Copy link
Contributor

yuvalif commented Oct 9, 2023

@yuvalif yuvalif merged commit d05fa0b into ceph:main Oct 9, 2023
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants