[tarantool-patches] Re: [PATCH v4 05/20] refactoring: clear privilege managing triggers from exceptions

Georgy Kirichenko georgy at tarantool.org
Mon Sep 30 15:28:08 MSK 2019


Hi, the patch is mostly Ok, please see some comments below

On Monday, September 23, 2019 6:56:56 PM MSK Ilya Kosarev wrote:
>  /**
> @@ -708,8 +710,15 @@ public:
>  static struct trigger *
>  txn_alter_trigger_new(trigger_f run, void *data)
>  {
> +	size_t size = sizeof(struct trigger);
>  	struct trigger *trigger = (struct trigger *)
> -		region_calloc_object_xc(&in_txn()->region, struct 
trigger);
> +		region_aligned_alloc(&in_txn()->region, size,
> +				     alignof(struct trigger));
> +	if (trigger == NULL) {
> +		diag_set(OutOfMemory, size, "region", "new slab");
Please use a structure name instead of a 'new slab' abbreviation (you can see 
an example after OutOfMemory search)
> +		return NULL;
> +	}
> +	trigger = (struct trigger *)memset(trigger, 0, size);
>  	trigger->run = run;
>  	trigger->data = data;
>  	trigger->destroy = NULL;
> @@ -979,6 +988,8 @@ alter_space_do(struct txn_stmt *stmt, struct alter_space
> *alter) struct trigger *on_commit, *on_rollback;
>  	on_commit = txn_alter_trigger_new(alter_space_commit, alter);
>  	on_rollback = txn_alter_trigger_new(alter_space_rollback, alter);
> +	if (on_commit == NULL || on_rollback == NULL)
> +		diag_raise();
> 
>  	/* Create a definition of the new space. */
>  	space_dump_def(alter->old_space, &alter->key_list);
> @@ -1938,8 +1949,9 @@ on_replace_dd_space(struct trigger * /* trigger */,
> void *event) region);
>  		auto def_guard =
>  			make_scoped_guard([=] { 
space_def_delete(def); });
> -		access_check_ddl(def->name, def->id, def->uid, 
SC_SPACE,
> -				 PRIV_C);
> +		if (access_check_ddl(def->name, def->id, def->uid, 
SC_SPACE,
> +				 PRIV_C) != 0)
> +			return -1;
>  		RLIST_HEAD(empty_list);
>  		struct space *space = space_new_xc(def, &empty_list);
>  		/**
> @@ -1965,6 +1977,8 @@ on_replace_dd_space(struct trigger * /* trigger */,
> void *event) */
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_create_space_rollback, space);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  		if (def->opts.is_view) {
>  			struct Select *select = 
sql_view_compile(sql_get(),
> @@ -1990,16 +2004,21 @@ on_replace_dd_space(struct trigger * /* trigger */,
> void *event) struct trigger *on_commit_view =
>  				
txn_alter_trigger_new(on_create_view_commit,
>  						      
select);
> +			if (on_commit_view == NULL)
> +				return -1;
>  			txn_stmt_on_commit(stmt, on_commit_view);
>  			struct trigger *on_rollback_view =
>  				
txn_alter_trigger_new(on_create_view_rollback,
>  						      
select);
> +			if (on_rollback_view == NULL)
> +				return -1;
>  			txn_stmt_on_rollback(stmt, 
on_rollback_view);
>  			select_guard.is_active = false;
>  		}
>  	} else if (new_tuple == NULL) { /* DELETE */
> -		access_check_ddl(old_space->def->name, old_space->def-
>id,
> -				 old_space->def->uid, SC_SPACE, 
PRIV_D);
> +		if (access_check_ddl(old_space->def->name, old_space-
>def->id,
> +				 old_space->def->uid, SC_SPACE, 
PRIV_D) != 0)
> +			return -1;
>  		/* Verify that the space is empty (has no indexes) */
>  		if (old_space->index_count) {
>  			diag_set(ClientError, ER_DROP_SPACE,
> @@ -2058,9 +2077,13 @@ on_replace_dd_space(struct trigger * /* trigger */,
> void *event) ++schema_version;
>  		struct trigger *on_commit =
>  			txn_alter_trigger_new(on_drop_space_commit, 
old_space);
> +		if (on_commit == NULL)
> +			return -1;
>  		txn_stmt_on_commit(stmt, on_commit);
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_drop_space_rollback, old_space);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  		if (old_space->def->opts.is_view) {
>  			struct Select *select =
> @@ -2074,10 +2097,14 @@ on_replace_dd_space(struct trigger * /* trigger */,
> void *event) struct trigger *on_commit_view =
>  				
txn_alter_trigger_new(on_drop_view_commit,
>  						      
select);
> +			if (on_commit_view == NULL)
> +				return -1;
>  			txn_stmt_on_commit(stmt, on_commit_view);
>  			struct trigger *on_rollback_view =
>  				
txn_alter_trigger_new(on_drop_view_rollback,
>  						      
select);
> +			if (on_rollback_view == NULL)
> +				return -1;
>  			txn_stmt_on_rollback(stmt, 
on_rollback_view);
>  			update_view_references(select, -1, true, 
NULL);
>  			select_guard.is_active = false;
> @@ -2089,8 +2116,9 @@ on_replace_dd_space(struct trigger * /* trigger */,
> void *event) region);
>  		auto def_guard =
>  			make_scoped_guard([=] { 
space_def_delete(def); });
> -		access_check_ddl(def->name, def->id, def->uid, 
SC_SPACE,
> -				 PRIV_A);
> +		if (access_check_ddl(def->name, def->id, def->uid, 
SC_SPACE,
> +				 PRIV_A) != 0)
> +			return -1;
>  		if (def->id != space_id(old_space)) {
>  			diag_set(ClientError, ER_ALTER_SPACE,
>  				  space_name(old_space),
> @@ -2246,8 +2274,9 @@ on_replace_dd_index(struct trigger * /* trigger */,
> void *event) enum priv_type priv_type = new_tuple ? PRIV_C : PRIV_D;
>  	if (old_tuple && new_tuple)
>  		priv_type = PRIV_A;
> -	access_check_ddl(old_space->def->name, old_space->def->id,
> -			 old_space->def->uid, SC_SPACE, priv_type);
> +	if (access_check_ddl(old_space->def->name, old_space->def->id,
> +			 old_space->def->uid, SC_SPACE, priv_type) != 
0)
> +		return -1;
>  	struct index *old_index = space_index(old_space, iid);
> 
>  	/*
> @@ -2684,8 +2713,9 @@ on_replace_dd_user(struct trigger * /* trigger */,
> void *event) struct user *old_user = user_by_id(uid);
>  	if (new_tuple != NULL && old_user == NULL) { /* INSERT */
>  		struct user_def *user = 
user_def_new_from_tuple(new_tuple);
> -		access_check_ddl(user->name, user->uid, user->owner, 
user->type,
> -				 PRIV_C);
> +		if (access_check_ddl(user->name, user->uid, user->owner, 
user->type,
> +				 PRIV_C) != 0)
> +			return -1;
>  		auto def_guard = make_scoped_guard([=] { free(user); 
});
>  		try {
>  			(void) user_cache_replace(user);
> @@ -2695,11 +2725,14 @@ on_replace_dd_user(struct trigger * /* trigger */,
> void *event) def_guard.is_active = false;
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(user_cache_remove_user, new_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  	} else if (new_tuple == NULL) { /* DELETE */
> -		access_check_ddl(old_user->def->name, old_user->def-
>uid,
> +		if (access_check_ddl(old_user->def->name, old_user->def-
>uid,
>  				 old_user->def->owner, old_user-
>def->type,
> -				 PRIV_D);
> +				 PRIV_D) != 0)
> +			return -1;
>  		/* Can't drop guest or super user */
>  		if (uid <= (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid == 
SUPER) {
>  			diag_set(ClientError, ER_DROP_USER,
> @@ -2719,6 +2752,8 @@ on_replace_dd_user(struct trigger * /* trigger */,
> void *event) user_cache_delete(uid);
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(user_cache_alter_user, 
old_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  	} else { /* UPDATE, REPLACE */
>  		assert(old_user != NULL && new_tuple != NULL);
> @@ -2728,8 +2763,9 @@ on_replace_dd_user(struct trigger * /* trigger */,
> void *event) * correct.
>  		 */
>  		struct user_def *user = 
user_def_new_from_tuple(new_tuple);
> -		access_check_ddl(user->name, user->uid, user->uid,
> -			         old_user->def->type, PRIV_A);
> +		if (access_check_ddl(user->name, user->uid, user->uid,
> +				 old_user->def->type, PRIV_A) != 
0)
> +			return -1;
>  		auto def_guard = make_scoped_guard([=] { free(user); 
});
>  		try {
>  			user_cache_replace(user);
> @@ -2739,6 +2775,8 @@ on_replace_dd_user(struct trigger * /* trigger */,
> void *event) def_guard.is_active = false;
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(user_cache_alter_user, 
old_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  	}
>  	return 0;
> @@ -2984,10 +3022,13 @@ on_replace_dd_func(struct trigger * /* trigger */,
> void *event) if (new_tuple != NULL && old_func == NULL) { /* INSERT */
>  		struct func_def *def = 
func_def_new_from_tuple(new_tuple);
>  		auto def_guard = make_scoped_guard([=] { free(def); });
> -		access_check_ddl(def->name, def->fid, def->uid, 
SC_FUNCTION,
> -				 PRIV_C);
> +		if (access_check_ddl(def->name, def->fid, def->uid, 
SC_FUNCTION,
> +				 PRIV_C) != 0)
> +			return -1;
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_create_func_rollback, NULL);
> +		if (on_rollback == NULL)
> +			return -1;
>  		struct func *func = func_new(def);
>  		if (func == NULL)
>  			return -1;
> @@ -3003,8 +3044,9 @@ on_replace_dd_func(struct trigger * /* trigger */,
> void *event) * Can only delete func if you're the one
>  		 * who created it or a superuser.
>  		 */
> -		access_check_ddl(old_func->def->name, fid, uid, 
SC_FUNCTION,
> -				 PRIV_D);
> +		if (access_check_ddl(old_func->def->name, fid, uid, 
SC_FUNCTION,
> +				 PRIV_D) != 0)
> +			return -1;
>  		/* Can only delete func if it has no grants. */
>  		if (schema_find_grants("function", old_func->def->fid)) {
>  			diag_set(ClientError, ER_DROP_FUNCTION,
> @@ -3030,6 +3072,8 @@ on_replace_dd_func(struct trigger * /* trigger */,
> void *event) txn_alter_trigger_new(on_drop_func_commit, old_func);
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(on_drop_func_rollback, 
old_func);
> +		if (on_commit == NULL || on_rollback == NULL)
> +			return -1;
>  		func_cache_delete(old_func->def->fid);
>  		txn_stmt_on_commit(stmt, on_commit);
>  		txn_stmt_on_rollback(stmt, on_rollback);
> @@ -3193,6 +3237,8 @@ on_replace_dd_collation(struct trigger * /* trigger
> */, void *event) txn_alter_trigger_new(on_drop_collation_commit, NULL);
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_drop_collation_rollback, NULL);
> +		if (on_commit == NULL || on_rollback == NULL)
> +			return -1;
>  		/*
>  		 * TODO: Check that no index uses the collation
>  		 * identifier.
> @@ -3212,9 +3258,10 @@ on_replace_dd_collation(struct trigger * /* trigger
> */, void *event) }
>  		struct coll_id *old_coll_id = coll_by_id(old_id);
>  		assert(old_coll_id != NULL);
> -		access_check_ddl(old_coll_id->name, old_coll_id->id,
> +		if (access_check_ddl(old_coll_id->name, old_coll_id->id,
>  				 old_coll_id->owner_id, 
SC_COLLATION,
> -				 PRIV_D);
> +				 PRIV_D) != 0)
> +			return -1;
>  		/*
>  		 * Set on_commit/on_rollback triggers after
>  		 * deletion from the cache to make trigger logic
> @@ -3229,10 +3276,13 @@ on_replace_dd_collation(struct trigger * /* trigger
> */, void *event) /* INSERT */
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_create_collation_rollback, NULL);
> +		if (on_rollback == NULL)
> +			return -1;
>  		struct coll_id_def new_def;
>  		coll_id_def_new_from_tuple(new_tuple, &new_def);
> -		access_check_ddl(new_def.name, new_def.id, 
new_def.owner_id,
> -				 SC_COLLATION, PRIV_C);
> +		if (access_check_ddl(new_def.name, new_def.id, 
new_def.owner_id,
> +				 SC_COLLATION, PRIV_C) != 0)
> +			return -1;
>  		struct coll_id *new_coll_id = coll_id_new(&new_def);
>  		if (new_coll_id == NULL)
>  			return -1;
> @@ -3256,20 +3306,25 @@ on_replace_dd_collation(struct trigger * /* trigger
> */, void *event) /**
>   * Create a privilege definition from tuple.
>   */
> -void
> +int
>  priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>  {
> -	priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
> -	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
> +	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ID, &(priv->grantor_id)) !
= 0)
> +		return -1;
> +	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &(priv->grantee_id)) 
!= 0)
> +		return -1;
> 
>  	const char *object_type =
> -		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
> +		tuple_field_cstr(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
> +	if (object_type == NULL)
> +		return -1;
>  	priv->object_type = schema_object_type(object_type);
> 
>  	const char *data = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>  	if (data == NULL) {
> -		tnt_raise(ClientError, ER_NO_SUCH_FIELD_NO,
> +		diag_set(ClientError, ER_NO_SUCH_FIELD_NO,
>  			  BOX_PRIV_FIELD_OBJECT_ID + 
TUPLE_INDEX_BASE);
> +		return -1;
>  	}
>  	/*
>  	 * When granting or revoking privileges on a whole entity
> @@ -3287,14 +3342,20 @@ priv_def_create_from_tuple(struct priv_def *priv,
> struct tuple *tuple) }
>  		FALLTHROUGH;
>  	default:
> -		priv->object_id = tuple_field_u32_xc(tuple,
> -						     
BOX_PRIV_FIELD_OBJECT_ID);
> +		if (tuple_field_u32(tuple,
> +		    BOX_PRIV_FIELD_OBJECT_ID, &(priv->object_id)) != 0)
> +			return -1;
>  	}
>  	if (priv->object_type == SC_UNKNOWN) {
> -		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
> +		diag_set(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
>  			  object_type);
> +		return -1;
>  	}
> -	priv->access = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ACCESS);
> +	uint32_t out;
> +	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ACCESS, &out) != 0)
> +		return -1;
> +	priv->access = out;
> +	return 0;
>  }
> 
>  /*
> @@ -3307,62 +3368,80 @@ priv_def_create_from_tuple(struct priv_def *priv,
> struct tuple *tuple) * object can be changed during WAL write.
>   * In the future we must protect grant/revoke with a logical lock.
>   */
> -static void
> +static int
>  priv_def_check(struct priv_def *priv, enum priv_type priv_type)
>  {
> -	struct user *grantor = user_find_xc(priv->grantor_id);
> +	struct user *grantor = user_find(priv->grantor_id);
> +	if (grantor == NULL)
> +		return -1;
>  	/* May be a role */
>  	struct user *grantee = user_by_id(priv->grantee_id);
>  	if (grantee == NULL) {
> -		tnt_raise(ClientError, ER_NO_SUCH_USER,
> +		diag_set(ClientError, ER_NO_SUCH_USER,
>  			  int2str(priv->grantee_id));
> +		return -1;
>  	}
>  	const char *name = schema_find_name(priv->object_type, priv-
>object_id);
> -	access_check_ddl(name, priv->object_id, grantor->def->uid,
> -			 priv->object_type, priv_type);
> +	if (access_check_ddl(name, priv->object_id, grantor->def->uid,
> +			     priv->object_type, priv_type) != 0)
> +		return -1;
>  	switch (priv->object_type) {
>  	case SC_UNIVERSE:
>  		if (grantor->def->uid != ADMIN) {
> -			tnt_raise(AccessDeniedError,
> +			diag_set(AccessDeniedError,
>  				  priv_name(priv_type),
>  				  
schema_object_name(SC_UNIVERSE),
>  				  name,
>  				  grantor->def->name);
> +			return -1;
>  		}
>  		break;
>  	case SC_SPACE:
>  	{
> -		struct space *space = space_cache_find_xc(priv-
>object_id);
> +		struct space *space = space_cache_find(priv->object_id);
> +		if (space == NULL)
> +			return -1;
>  		if (space->def->uid != grantor->def->uid &&
>  		    grantor->def->uid != ADMIN) {
> -			tnt_raise(AccessDeniedError,
> +			diag_set(AccessDeniedError,
>  				  priv_name(priv_type),
>  				  schema_object_name(SC_SPACE), 
name,
>  				  grantor->def->name);
> +			return -1;
>  		}
>  		break;
>  	}
>  	case SC_FUNCTION:
>  	{
> -		struct func *func = func_cache_find(priv->object_id);
> +		struct func *func = func_by_id(priv->object_id);
> +		if (func == NULL) {
> +			diag_set(ClientError, ER_NO_SUCH_FUNCTION, 
int2str(priv->object_id));
> +			return -1;
> +		}
>  		if (func->def->uid != grantor->def->uid &&
>  		    grantor->def->uid != ADMIN) {
> -			tnt_raise(AccessDeniedError,
> +			diag_set(AccessDeniedError,
>  				  priv_name(priv_type),
>  				  
schema_object_name(SC_FUNCTION), name,
>  				  grantor->def->name);
> +			return -1;
>  		}
>  		break;
>  	}
>  	case SC_SEQUENCE:
>  	{
> -		struct sequence *seq = sequence_cache_find(priv-
>object_id);
> +		struct sequence *seq = sequence_by_id(priv->object_id);
> +		if (seq == NULL) {
> +			diag_set(ClientError, ER_NO_SUCH_SEQUENCE, 
int2str(priv->object_id));
> +			return -1;
> +		}
>  		if (seq->def->uid != grantor->def->uid &&
>  		    grantor->def->uid != ADMIN) {
> -			tnt_raise(AccessDeniedError,
> +			diag_set(AccessDeniedError,
>  				  priv_name(priv_type),
>  				  
schema_object_name(SC_SEQUENCE), name,
>  				  grantor->def->name);
> +			return -1;
>  		}
>  		break;
>  	}
> @@ -3370,9 +3449,10 @@ priv_def_check(struct priv_def *priv, enum priv_type
> priv_type) {
>  		struct user *role = user_by_id(priv->object_id);
>  		if (role == NULL || role->def->type != SC_ROLE) {
> -			tnt_raise(ClientError, ER_NO_SUCH_ROLE,
> +			diag_set(ClientError, ER_NO_SUCH_ROLE,
>  				  role ? role->def->name :
>  				  int2str(priv->object_id));
> +			return -1;
>  		}
>  		/*
>  		 * Only the creator of the role can grant or revoke it.
> @@ -3381,29 +3461,33 @@ priv_def_check(struct priv_def *priv, enum priv_type
> priv_type) if (role->def->owner != grantor->def->uid &&
>  		    grantor->def->uid != ADMIN &&
>  		    (role->def->uid != PUBLIC || priv->access != 
PRIV_X)) {
> -			tnt_raise(AccessDeniedError,
> +			diag_set(AccessDeniedError,
>  				  priv_name(priv_type),
>  				  schema_object_name(SC_ROLE), 
name,
>  				  grantor->def->name);
> +			return -1;
>  		}
>  		/* Not necessary to do during revoke, but who cares. */
> -		role_check(grantee, role);
> +		if (role_check(grantee, role) != 0)
> +			return -1;
>  		break;
>  	}
>  	case SC_USER:
>  	{
>  		struct user *user = user_by_id(priv->object_id);
>  		if (user == NULL || user->def->type != SC_USER) {
> -			tnt_raise(ClientError, ER_NO_SUCH_USER,
> +			diag_set(ClientError, ER_NO_SUCH_USER,
>  				  user ? user->def->name :
>  				  int2str(priv->object_id));
> +			return -1;
>  		}
>  		if (user->def->owner != grantor->def->uid &&
>  		    grantor->def->uid != ADMIN) {
> -			tnt_raise(AccessDeniedError,
> +			diag_set(AccessDeniedError,
>  				  priv_name(priv_type),
>  				  schema_object_name(SC_USER), 
name,
>  				  grantor->def->name);
> +			return -1;
>  		}
>  		break;
>  	}
> @@ -3415,30 +3499,33 @@ priv_def_check(struct priv_def *priv, enum priv_type
> priv_type) {
>  		/* Only admin may grant privileges on an entire entity. 
*/
>  		if (grantor->def->uid != ADMIN) {
> -			tnt_raise(AccessDeniedError, 
priv_name(priv_type),
> +			diag_set(AccessDeniedError, 
priv_name(priv_type),
>  				  schema_object_name(priv-
>object_type), name,
>  				  grantor->def->name);
> +			return -1;
>  		}
>  	}
>  	default:
>  		break;
>  	}
>  	if (priv->access == 0) {
> -		tnt_raise(ClientError, ER_GRANT,
> +		diag_set(ClientError, ER_GRANT,
>  			  "the grant tuple has no privileges");
> +		return -1;
>  	}
> +	return 0;
>  }
> 
>  /**
>   * Update a metadata cache object with the new access
>   * data.
>   */
> -static void
> +static int
>  grant_or_revoke(struct priv_def *priv)
>  {
>  	struct user *grantee = user_by_id(priv->grantee_id);
>  	if (grantee == NULL)
> -		return;
> +		return 0;
>  	/*
>  	 * Grant a role to a user only when privilege type is 'execute'
>  	 * and the role is specified.
> @@ -3446,14 +3533,19 @@ grant_or_revoke(struct priv_def *priv)
>  	if (priv->object_type == SC_ROLE && !(priv->access & ~PRIV_X)) {
>  		struct user *role = user_by_id(priv->object_id);
>  		if (role == NULL || role->def->type != SC_ROLE)
> -			return;
> -		if (priv->access)
> -			role_grant(grantee, role);
> -		else
> -			role_revoke(grantee, role);
> +			return 0;
> +		if (priv->access) {
> +			if (role_grant(grantee, role) != 0)
> +				return -1;
> +		} else {
> +			if (role_revoke(grantee, role) != 0)
> +				return -1;
> +		}
>  	} else {
> -		priv_grant(grantee, priv);
> +		if (priv_grant(grantee, priv) != 0)
> +			return -1;
>  	}
> +	return 0;
>  }
> 
>  /** A trigger called on rollback of grant. */
> @@ -3463,9 +3555,11 @@ revoke_priv(struct trigger *trigger, void *event)
>  	(void) event;
>  	struct tuple *tuple = (struct tuple *)trigger->data;
>  	struct priv_def priv;
> -	priv_def_create_from_tuple(&priv, tuple);
> +	if (priv_def_create_from_tuple(&priv, tuple) != 0)
> +		return -1;
>  	priv.access = 0;
> -	grant_or_revoke(&priv);
> +	if (grant_or_revoke(&priv) != 0)
> +		return -1;
>  	return 0;
>  }
> 
> @@ -3476,8 +3570,10 @@ modify_priv(struct trigger *trigger, void *event)
>  	(void) event;
>  	struct tuple *tuple = (struct tuple *)trigger->data;
>  	struct priv_def priv;
> -	priv_def_create_from_tuple(&priv, tuple);
> -	grant_or_revoke(&priv);
> +	if (priv_def_create_from_tuple(&priv, tuple) != 0)
> +		return -1;
> +	if (grant_or_revoke(&priv) != 0)
> +		return -1;
>  	return 0;
>  }
> 
> @@ -3495,27 +3591,42 @@ on_replace_dd_priv(struct trigger * /* trigger */,
> void *event) struct priv_def priv;
> 
>  	if (new_tuple != NULL && old_tuple == NULL) {	/* grant */
> -		priv_def_create_from_tuple(&priv, new_tuple);
> -		priv_def_check(&priv, PRIV_GRANT);
> -		grant_or_revoke(&priv);
> +		if (priv_def_create_from_tuple(&priv, new_tuple) != 0)
> +			return -1;
> +		if (priv_def_check(&priv, PRIV_GRANT) != 0)
> +			return -1;
> +		if (grant_or_revoke(&priv) != 0)
> +			return -1;
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(revoke_priv, 
new_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  	} else if (new_tuple == NULL) {                /* revoke */
>  		assert(old_tuple);
> -		priv_def_create_from_tuple(&priv, old_tuple);
> -		priv_def_check(&priv, PRIV_REVOKE);
> +		if (priv_def_create_from_tuple(&priv, old_tuple) != 0)
> +			return -1;
> +		if (priv_def_check(&priv, PRIV_REVOKE) != 0)
> +			return -1;
It would be looking better if you combine such blocks into an or condition 
like:
if (priv_def_create_from_tuple(&priv, old_tuple) != 0 ||
    priv_def_check(&priv, PRIV_REVOKE) != 0)
       return -1;

>  		priv.access = 0;
> -		grant_or_revoke(&priv);
> +		if (grant_or_revoke(&priv) != 0)
> +			return -1;
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(modify_priv, 
old_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  	} else {                                       /* modify */
> -		priv_def_create_from_tuple(&priv, new_tuple);
> -		priv_def_check(&priv, PRIV_GRANT);
> -		grant_or_revoke(&priv);
> +		if (priv_def_create_from_tuple(&priv, new_tuple) != 0)
> +			return -1;
> +		if (priv_def_check(&priv, PRIV_GRANT) != 0)
> +			return -1;
> +		if (grant_or_revoke(&priv) != 0)
> +			return -1;
Please combine it into one condition here and other places
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(modify_priv, 
old_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  	}
>  	return 0;
> @@ -3656,6 +3767,8 @@ on_replace_dd_cluster(struct trigger *trigger, void
> *event) struct trigger *on_commit;
>  			on_commit = 
txn_alter_trigger_new(register_replica,
>  							  
new_tuple);
> +			if (on_commit == NULL)
> +				return -1;
>  			txn_stmt_on_commit(stmt, on_commit);
>  		}
>  	} else {
> @@ -3672,6 +3785,8 @@ on_replace_dd_cluster(struct trigger *trigger, void
> *event) struct trigger *on_commit;
>  		on_commit = txn_alter_trigger_new(unregister_replica,
>  						  
old_tuple);
> +		if (on_commit == NULL)
> +			return -1;
>  		txn_stmt_on_commit(stmt, on_commit);
>  	}
>  	return 0;
> @@ -3794,10 +3909,13 @@ on_replace_dd_sequence(struct trigger * /* trigger
> */, void *event) if (old_tuple == NULL && new_tuple != NULL) {		/
* INSERT
> */
>  		new_def = sequence_def_new_from_tuple(new_tuple,
>  						      
ER_CREATE_SEQUENCE);
> -		access_check_ddl(new_def->name, new_def->id, new_def-
>uid,
> -				 SC_SEQUENCE, PRIV_C);
> +		if (access_check_ddl(new_def->name, new_def->id, 
new_def->uid,
> +				 SC_SEQUENCE, PRIV_C) != 0)
> +			return -1;
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_create_sequence_rollback, NULL);
> +		if (on_rollback == NULL)
> +			return -1;
>  		seq = sequence_new_xc(new_def);
>  		sequence_cache_insert(seq);
>  		on_rollback->data = seq;
> @@ -3807,8 +3925,9 @@ on_replace_dd_sequence(struct trigger * /* trigger */,
> void *event) BOX_SEQUENCE_DATA_FIELD_ID);
>  		seq = sequence_by_id(id);
>  		assert(seq != NULL);
> -		access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> -				 SC_SEQUENCE, PRIV_D);
> +		if (access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> +				 SC_SEQUENCE, PRIV_D) != 0)
> +			return -1;
>  		if (space_has_data(BOX_SEQUENCE_DATA_ID, 0, id)) {
>  			diag_set(ClientError, ER_DROP_SEQUENCE,
>  				  seq->def->name, "the sequence 
has data");
> @@ -3828,6 +3947,8 @@ on_replace_dd_sequence(struct trigger * /* trigger */,
> void *event) txn_alter_trigger_new(on_drop_sequence_commit, seq);
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_drop_sequence_rollback, seq);
> +		if (on_commit == NULL || on_rollback == NULL)
> +			return -1;
>  		sequence_cache_delete(seq->def->id);
>  		txn_stmt_on_commit(stmt, on_commit);
>  		txn_stmt_on_rollback(stmt, on_rollback);
> @@ -3836,12 +3957,15 @@ on_replace_dd_sequence(struct trigger * /* trigger
> */, void *event) ER_ALTER_SEQUENCE);
>  		seq = sequence_by_id(new_def->id);
>  		assert(seq != NULL);
> -		access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> -				 SC_SEQUENCE, PRIV_A);
> +		if (access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> +				 SC_SEQUENCE, PRIV_A) != 0)
> +			return -1;
>  		struct trigger *on_commit =
>  			
txn_alter_trigger_new(on_alter_sequence_commit, seq->def);
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_alter_sequence_rollback, seq->def);
> +		if (on_commit == NULL || on_rollback == NULL)
> +			return -1;
>  		seq->def = new_def;
>  		txn_stmt_on_commit(stmt, on_commit);
>  		txn_stmt_on_rollback(stmt, on_rollback);
> @@ -3899,6 +4023,8 @@ on_replace_dd_sequence_data(struct trigger * /*
> trigger */, void *event) */
>  		struct trigger *on_rollback = txn_alter_trigger_new(
>  				on_drop_sequence_data_rollback, 
old_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  		sequence_reset(seq);
>  	}
> @@ -4015,21 +4141,25 @@ on_replace_dd_space_sequence(struct trigger * /*
> trigger */, void *event)
> 
>  	/* Check we have the correct access type on the sequence.  * */
>  	if (is_generated || !stmt->new_tuple) {
> -		access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> -				 SC_SEQUENCE, priv_type);
> +		if (access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> +				 SC_SEQUENCE, priv_type) != 0)
> +			return -1;
>  	} else {
>  		/*
>  		 * In case user wants to attach an existing sequence,
>  		 * check that it has read and write access.
>  		 */
> -		access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> -				 SC_SEQUENCE, PRIV_R);
> -		access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> -				 SC_SEQUENCE, PRIV_W);
> +		if (access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> +				 SC_SEQUENCE, PRIV_R) != 0)
> +			return -1;
> +		if (access_check_ddl(seq->def->name, seq->def->id, seq-
>def->uid,
> +				 SC_SEQUENCE, PRIV_W) != 0)
> +			return -1;
>  	}
>  	/** Check we have alter access on space. */
> -	access_check_ddl(space->def->name, space->def->id, space->def-
>uid,
> -			 SC_SPACE, PRIV_A);
> +	if (access_check_ddl(space->def->name, space->def->id, space->def-
>uid,
> +			 SC_SPACE, PRIV_A) != 0)
> +		return -1;
> 
>  	if (stmt->new_tuple != NULL) {			/* 
INSERT, UPDATE */
>  		char *sequence_path;
> @@ -4052,6 +4182,8 @@ on_replace_dd_space_sequence(struct trigger * /*
> trigger */, void *event) else
>  			on_rollback = 
txn_alter_trigger_new(clear_space_sequence,
>  							    
stmt->new_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		seq->is_generated = is_generated;
>  		space->sequence = seq;
>  		space->sequence_fieldno = sequence_fieldno;
> @@ -4063,6 +4195,8 @@ on_replace_dd_space_sequence(struct trigger * /*
> trigger */, void *event) struct trigger *on_rollback;
>  		on_rollback = txn_alter_trigger_new(set_space_sequence,
>  						    stmt-
>old_tuple);
> +		if (on_rollback == NULL)
> +			return -1;
>  		assert(space->sequence == seq);
>  		seq->is_generated = false;
>  		space->sequence = NULL;
> @@ -4153,6 +4287,8 @@ on_replace_dd_trigger(struct trigger * /* trigger */,
> void *event) struct trigger *on_rollback = txn_alter_trigger_new(NULL,
> NULL); struct trigger *on_commit =
>  		txn_alter_trigger_new(on_replace_trigger_commit, NULL);
> +	if (on_commit == NULL || on_rollback == NULL)
> +		return -1;
> 
>  	if (old_tuple != NULL && new_tuple == NULL) {
>  		/* DROP trigger. */
> @@ -4655,6 +4791,8 @@ on_replace_dd_fk_constraint(struct trigger * /*
> trigger*/, void *event) struct trigger *on_rollback =
>  				
txn_alter_trigger_new(on_create_fk_constraint_rollback,
>  						      fk);
> +			if (on_rollback == NULL)
> +				return -1;
>  			txn_stmt_on_rollback(stmt, on_rollback);
>  			fk_constraint_set_mask(fk,
>  					       &parent_space-
>fk_constraint_mask,
> @@ -4673,10 +4811,14 @@ on_replace_dd_fk_constraint(struct trigger * /*
> trigger*/, void *event) struct trigger *on_rollback =
>  				
txn_alter_trigger_new(on_replace_fk_constraint_rollback,
>  						      
old_fk);
> +			if (on_rollback == NULL)
> +				return -1;
>  			txn_stmt_on_rollback(stmt, on_rollback);
>  			struct trigger *on_commit =
>  				
txn_alter_trigger_new(on_drop_or_replace_fk_constraint_commit,
>  						      
old_fk);
> +			if (on_commit == NULL)
> +				return -1;
>  			txn_stmt_on_commit(stmt, on_commit);
>  			space_reset_fk_constraint_mask(child_space);
>  			
space_reset_fk_constraint_mask(parent_space);
> @@ -4699,10 +4841,14 @@ on_replace_dd_fk_constraint(struct trigger * /*
> trigger*/, void *event) struct trigger *on_commit =
>  			
txn_alter_trigger_new(on_drop_or_replace_fk_constraint_commit,
>  					      old_fk);
> +		if (on_commit == NULL)
> +			return -1;
>  		txn_stmt_on_commit(stmt, on_commit);
>  		struct trigger *on_rollback =
>  			
txn_alter_trigger_new(on_drop_fk_constraint_rollback,
>  					      old_fk);
> +		if (on_rollback == NULL)
> +			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  		space_reset_fk_constraint_mask(child_space);
>  		space_reset_fk_constraint_mask(parent_space);
> @@ -4830,6 +4976,8 @@ on_replace_dd_ck_constraint(struct trigger * /*
> trigger*/, void *event) struct space *space =
> space_cache_find_xc(space_id);
>  	struct trigger *on_rollback = txn_alter_trigger_new(NULL, NULL);
>  	struct trigger *on_commit = txn_alter_trigger_new(NULL, NULL);
> +	if (on_commit == NULL || on_rollback == NULL)
> +		return -1;
> 
>  	if (new_tuple != NULL) {
>  		bool is_deferred =
> diff --git a/src/box/user.cc b/src/box/user.cc
> index c46ff67d1..92a183c2a 100644
> --- a/src/box/user.cc
> +++ b/src/box/user.cc
> @@ -180,18 +180,24 @@ user_destroy(struct user *user)
>   * Add a privilege definition to the list
>   * of effective privileges of a user.
>   */
> -void
> +int
>  user_grant_priv(struct user *user, struct priv_def *def)
>  {
>  	struct priv_def *old = privset_search(&user->privs, def);
>  	if (old == NULL) {
> +		size_t size = sizeof(struct priv_def);
>  		old = (struct priv_def *)
> -			region_alloc_xc(&user->pool, sizeof(struct 
priv_def));
> +			region_alloc(&user->pool, size);
> +		if (old == NULL) {
> +			diag_set(OutOfMemory, size, "region", "new 
slab");
> +			return -1;
> +		}
>  		*old = *def;
>  		privset_insert(&user->privs, old);
>  	} else {
>  		old->access |= def->access;
>  	}
> +	return 0;
>  }
> 
>  /**
> @@ -305,11 +311,11 @@ user_set_effective_access(struct user *user)
>  /**
>   * Reload user privileges and re-grant them.
>   */
> -static void
> +static int
>  user_reload_privs(struct user *user)
>  {
>  	if (user->is_dirty == false)
> -		return;
> +		return 0;
>  	struct priv_def *priv;
>  	/**
>  	 * Reset effective access of the user in the
> @@ -326,26 +332,43 @@ user_reload_privs(struct user *user)
>  	privset_new(&user->privs);
>  	/* Load granted privs from _priv space. */
>  	{
> -		struct space *space = space_cache_find_xc(BOX_PRIV_ID);
> +		struct space *space = space_cache_find(BOX_PRIV_ID);
> +		if (space == NULL)
> +			return -1;
>  		char key[6];
>  		/** Primary key - by user id */
> -		struct index *index = index_find_system_xc(space, 0);
> +		if (!space_is_memtx(space)) {
> +			diag_set(ClientError, ER_UNSUPPORTED,
> +			          space->engine->name, "system 
data");
> +			return -1;
> +		}
> +		struct index *index = index_find(space, 0);
> +		if (index == NULL)
> +			return -1;
>  		mp_encode_uint(key, user->def->uid);
> 
> -		struct iterator *it = index_create_iterator_xc(index, 
ITER_EQ,
> +		struct iterator *it = index_create_iterator(index, 
ITER_EQ,
>  							       
key, 1);
> +		if (it == NULL)
> +			return -1;
>  		IteratorGuard iter_guard(it);
> 
>  		struct tuple *tuple;
> -		while ((tuple = iterator_next_xc(it)) != NULL) {
> +		if (iterator_next(it, &tuple) != 0)
> +			return -1;
> +		while (tuple != NULL) {
>  			struct priv_def priv;
> -			priv_def_create_from_tuple(&priv, tuple);
> +			if (priv_def_create_from_tuple(&priv, tuple) 
!= 0)
> +				return -1;
>  			/**
>  			 * Skip role grants, we're only
>  			 * interested in real objects.
>  			 */
>  			if (priv.object_type != SC_ROLE || !
(priv.access & PRIV_X))
> -				user_grant_priv(user, &priv);
> +				if (user_grant_priv(user, &priv) !
= 0)
> +					return -1;
> +			if (iterator_next(it, &tuple) != 0)
> +				return -1;
>  		}
>  	}
>  	{
> @@ -358,12 +381,14 @@ user_reload_privs(struct user *user)
>  			privset_ifirst(&role->privs, &it);
>  			struct priv_def *def;
>  			while ((def = privset_inext(&it))) {
> -				user_grant_priv(user, def);
> +				if (user_grant_priv(user, def) != 
0)
> +					return -1;
>  			}
>  		}
>  	}
>  	user_set_effective_access(user);
>  	user->is_dirty = false;
> +	return 0;
>  }
> 
>  /** }}} */
> @@ -559,7 +584,7 @@ user_cache_free()
> 
>  /** {{{ roles */
> 
> -void
> +int
>  role_check(struct user *grantee, struct user *role)
>  {
>  	/*
> @@ -592,16 +617,18 @@ role_check(struct user *grantee, struct user *role)
>  	 */
>  	if (user_map_is_set(&transitive_closure,
>  			    role->auth_token)) {
> -		tnt_raise(ClientError, ER_ROLE_LOOP,
> +		diag_set(ClientError, ER_ROLE_LOOP,
>  			  role->def->name, grantee->def->name);
> +		return -1;
>  	}
> +	return 0;
>  }
> 
>  /**
>   * Re-calculate effective grants of the linked subgraph
>   * this user/role is a part of.
>   */
> -void
> +int
>  rebuild_effective_grants(struct user *grantee)
>  {
>  	/*
> @@ -653,7 +680,8 @@ rebuild_effective_grants(struct user *grantee)
>  			struct user_map indirect_edges = user-
>roles;
>  			user_map_minus(&indirect_edges, 
&transitive_closure);
>  			if (user_map_is_empty(&indirect_edges)) {
> -				user_reload_privs(user);
> +				if (user_reload_privs(user) != 0)
> +					return -1;
>  				user_map_union(&next_layer, 
&user->users);
>  			} else {
>  				/*
> @@ -674,6 +702,7 @@ rebuild_effective_grants(struct user *grantee)
>  		user_map_union(&transitive_closure, &current_layer);
>  		current_layer = next_layer;
>  	}
> +	return 0;
>  }
> 
> 
> @@ -682,35 +711,41 @@ rebuild_effective_grants(struct user *grantee)
>   * Grant all effective privileges of the role to whoever
>   * this role was granted to.
>   */
> -void
> +int
>  role_grant(struct user *grantee, struct user *role)
>  {
>  	user_map_set(&role->users, grantee->auth_token);
>  	user_map_set(&grantee->roles, role->auth_token);
> -	rebuild_effective_grants(grantee);
> +	if (rebuild_effective_grants(grantee) != 0)
> +		return -1;
> +	return 0;
>  }
> 
>  /**
>   * Update the role dependencies graph.
>   * Rebuild effective privileges of the grantee.
>   */
> -void
> +int
>  role_revoke(struct user *grantee, struct user *role)
>  {
>  	user_map_clear(&role->users, grantee->auth_token);
>  	user_map_clear(&grantee->roles, role->auth_token);
> -	rebuild_effective_grants(grantee);
> +	if (rebuild_effective_grants(grantee) != 0)
> +		return -1;
> +	return 0;
>  }
> 
> -void
> +int
>  priv_grant(struct user *grantee, struct priv_def *priv)
>  {
>  	struct access *object = access_find(priv->object_type, priv-
>object_id);
>  	if (object == NULL)
> -		return;
> +		return 0;
>  	struct access *access = &object[grantee->auth_token];
>  	access->granted = priv->access;
> -	rebuild_effective_grants(grantee);
> +	if (rebuild_effective_grants(grantee) != 0)
> +		return -1;
> +	return 0;
>  }
> 
>  /** }}} */
> diff --git a/src/box/user.h b/src/box/user.h
> index 527fb2e7c..ae2b73e55 100644
> --- a/src/box/user.h
> +++ b/src/box/user.h
> @@ -144,16 +144,6 @@ user_cache_replace(struct user_def *user);
>  void
>  user_cache_delete(uint32_t uid);
> 
> -/* Find a user by name. Used by authentication. */
> -static inline struct user *
> -user_find_xc(uint32_t uid)
> -{
> -	struct user *user = user_find(uid);
> -	if (user == NULL)
> -		diag_raise();
> -	return user;
> -}
> -
>  static inline struct user *
>  user_find_by_name_xc(const char *name, uint32_t len)
>  {
> @@ -178,19 +168,19 @@ user_cache_free();
>   * and no loop in the graph will occur when grantee gets
>   * a given role.
>   */
> -void
> +int
>  role_check(struct user *grantee, struct user *role);
> 
>  /**
>   * Grant a role to a user or another role.
>   */
> -void
> +int
>  role_grant(struct user *grantee, struct user *role);
> 
>  /**
>   * Revoke a role from a user or another role.
>   */
> -void
> +int
>  role_revoke(struct user *grantee, struct user *role);
> 
>  /**
> @@ -198,10 +188,10 @@ role_revoke(struct user *grantee, struct user *role);
>   * and re-evaluate effective access of all users of this
>   * role if this role.
>   */
> -void
> +int
>  priv_grant(struct user *grantee, struct priv_def *priv);
> 
> -void
> +int
>  priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple);
> 
>  /* }}} */

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190930/5ecfd2e3/attachment.sig>


More information about the Tarantool-patches mailing list