Skip to content

Commit

Permalink
#1064: Fix data race with YQL permissions cache
Browse files Browse the repository at this point in the history
Summary: Ensure that before accessing the cache, we increase the refcount so that it doesn't get deallocated in case the cache's pointer gets updated by a background thread.

Test Plan: ybd tsan --java-test org.yb.cql.TestAuthorizationEnforcement#testGrantRoleWithPermissionOnGrantedAndRecipientRoles -n 100

Reviewers: mikhail, bogdan, robert

Reviewed By: robert

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D6393
  • Loading branch information
hectorgcr committed Mar 27, 2019
1 parent 4a56fe1 commit 9d333f7
Showing 1 changed file with 11 additions and 4 deletions.
15 changes: 11 additions & 4 deletions src/yb/client/permissions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
// under the License.
//

#include <atomic>

#include "yb/client/client.h"
#include "yb/client/permissions.h"

Expand Down Expand Up @@ -105,7 +107,7 @@ void PermissionsCache::ScheduleGetPermissionsFromMaster(bool now) {
}

void PermissionsCache::UpdateRolesPermissions(const GetPermissionsResponsePB& resp) {
auto new_roles_permissions_map = std::make_unique<RolesPermissionsMap>();
auto new_roles_permissions_map = std::make_shared<RolesPermissionsMap>();

// Populate the cache.
// Get all the roles in the response. They should have at least one piece of information:
Expand All @@ -121,7 +123,8 @@ void PermissionsCache::UpdateRolesPermissions(const GetPermissionsResponsePB& re
std::unique_lock<simple_spinlock> l(permissions_cache_lock_);
// It's possible that another thread already updated the cache with a more recent version.
if (version_ < resp.version()) {
roles_permissions_map_ = std::move(new_roles_permissions_map);
std::atomic_store_explicit(&roles_permissions_map_, std::move(new_roles_permissions_map),
std::memory_order_release);
// Set the permissions cache's version.
version_ = resp.version();
}
Expand Down Expand Up @@ -155,8 +158,12 @@ bool PermissionsCache::HasCanonicalResourcePermission(const std::string& canonic
const ql::ObjectType& object_type,
const RoleName& role_name,
const PermissionType& permission) {
const auto& role_permissions_iter = roles_permissions_map_->find(role_name);
if (role_permissions_iter == roles_permissions_map_->end()) {
std::shared_ptr<RolesPermissionsMap> roles_permissions_map;
roles_permissions_map = std::atomic_load_explicit(&roles_permissions_map_,
std::memory_order_acquire);

const auto& role_permissions_iter = roles_permissions_map->find(role_name);
if (role_permissions_iter == roles_permissions_map->end()) {
VLOG(1) << "Role " << role_name << " not found";
// Role doesn't exist.
return false;
Expand Down

0 comments on commit 9d333f7

Please sign in to comment.