rgw: Guard against malformed bucket URLs

[adamemerson]

Misplaced colons can result in radosgw thinking is has a bucket URL
but with no bucket name, leading to a crash later on.

Fixes: https://tracker.ceph.com/issues/55765

Contribution Guidelines

• To sign and title your commits, please refer to Submitting Patches to Ceph.

• If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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)
  • Includes unit test(s)
  • Includes integration test(s)
  • Includes bug reproducer
  • No tests

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

[cbodley]

@adamemerson can you clarify where exactly the crash happens? is it really inside of rgw::ARN::ARN(rgw_bucket const&)?

[adamemerson]

@adamemerson can you clarify where exactly the crash happens? is it really inside of rgw::ARN::ARN(rgw_bucket const&)?

No, in its parameter list.

With the invalid bucket URL, where there was a colon followed something terminating the bucket parse, s->bucket in ARN(s->bucket->get_key())) would end up being null.

[adamemerson]

I'm guessing the crash got introduced in 99f7c4a when req_state::bucket went from being an rgw_bucket object to a unique_ptr .

[cbodley]

thanks @adamemerson, that makes sense! i like your fix, but i'd also like to see an explicit null check. the top-level overload of verify_bucket_permission() looks like a good place:

 bool verify_bucket_permission(const DoutPrefixProvider* dpp, struct req_state * const s, const uint64_t op)
 {
+  if (rgw::sal::Bucket::empty(s->bucket)) {
+    return false; // request is missing a bucket name
+  }
   perm_state_from_req_state ps(s);
 
   return verify_bucket_permission(dpp,
                                   &ps,
                                   s->bucket->get_key(),

cc @dang

[adamemerson]

@cbodley Took out the top level check since s3tests wouldn't even initialize with it there.

[cbodley]

cc @ivancich, there was a force-push after you labeled this for testing

[adamemerson]

http://pulpito.ceph.com/aemerson-2022-07-16_13:14:31-rgw-wip-55765-distro-default-smithi/