From c239235e863f602d6f0e4ccd3289721b8cf82947 Mon Sep 17 00:00:00 2001
From: Roman Tsisyk <roman@tsisyk.com>
Date: Thu, 26 Nov 2015 16:20:27 +0300
Subject: [PATCH] Refactor box/lua/call.cc
* Extract high-level IPROTO_CALL code to box.cc
* Move execute_c_call to func.cc
* Prepare src/lua/call.cc to rewrite in C
Incompatible changes:
* #300 logic was reverted. An attempt to call an unexisting Lua function
without universal execute permissions now cause "access denied" message
instead of "function does not exist".
Since there is no cache for Lua procedures, existence of a function can be
checked only by attempt to execute it. Caller **must** have a permission
to execute function in order to execute functions. It's obvious, isn't it?
Anyway, this patch doesn't affect user experience with using stored
procedures. The two steps still must be performed to allow Lua calls
using the binary protocol:
1. A function must be defined in Lua
2. A function must be declared in box.schema and caller must have
execute permission for this object or for entire "universe".
The order actually doesn't matter - the both steps must be done.
---
src/box/box.cc | 90 +++++++++++++++++++++
src/box/box.h | 4 +
src/box/func.cc | 57 +++++++++++++
src/box/func.h | 15 ++++
src/box/iproto.cc | 12 ++-
src/box/iproto_port.cc | 20 +++--
src/box/iproto_port.h | 4 +-
src/box/lua/call.cc | 170 ++++-----------------------------------
src/box/lua/call.h | 4 +-
src/box/tuple.h | 2 +-
src/box/tuple_convert.cc | 8 +-
test/box/access.result | 4 +-
test/box/net.box.result | 8 +-
13 files changed, 222 insertions(+), 176 deletions(-)
diff --git a/src/box/box.cc b/src/box/box.cc
index c1fd9216cd..f0b8548b5a 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -40,6 +40,8 @@
#include "main.h"
#include "tuple.h"
#include "session.h"
+#include "user.h"
+#include "func.h"
#include "schema.h"
#include "engine.h"
#include "memtx_engine.h"
@@ -56,6 +58,7 @@
#include "coio.h"
#include "cluster.h" /* replica */
#include "title.h"
+#include "lua/call.h" /* box_lua_call */
static char status[64] = "unknown";
@@ -665,6 +668,93 @@ box_on_cluster_join(const tt_uuid *server_uuid)
(unsigned) server_id, tt_uuid_str(server_uuid));
}
+static inline struct func *
+access_check_func(const char *name, uint32_t name_len)
+{
+ struct func *func = func_by_name(name, name_len);
+ struct credentials *credentials = current_user();
+
+ /*
+ * If the user has universal access, don't bother with checks.
+ * No special check for ADMIN user is necessary
+ * since ADMIN has universal access.
+ */
+ if ((credentials->universal_access & PRIV_ALL) == PRIV_ALL)
+ return func;
+ uint8_t access = PRIV_X & ~credentials->universal_access;
+ if (func == NULL || (func->def.uid != credentials->uid &&
+ access & ~func->access[credentials->auth_token].effective)) {
+ /* Access violation, report error. */
+ char name_buf[BOX_NAME_MAX + 1];
+ snprintf(name_buf, sizeof(name_buf), "%.*s", name_len, name);
+ struct user *user = user_find_xc(credentials->uid);
+ tnt_raise(ClientError, ER_FUNCTION_ACCESS_DENIED,
+ priv_name(access), user->def.name, name_buf);
+ }
+
+ return func;
+}
+
+void
+box_process_call(struct request *request, struct obuf *out)
+{
+ /**
+ * Find the function definition and check access.
+ */
+ const char *name = request->key;
+ uint32_t name_len = mp_decode_strl(&name);
+ struct func *func = access_check_func(name, name_len);
+ /*
+ * Sic: func == NULL means that perhaps the user has a global
+ * "EXECUTE" privilege, so no specific grant to a function.
+ */
+
+ /**
+ * Change the current user id if the function is a set-definer-uid one.
+ */
+ struct credentials *orig_credentials = NULL;
+ if (func && func->def.setuid) {
+ orig_credentials = current_user();
+ /** Remember and change the current user id. */
+ if (func->owner_credentials.auth_token >= BOX_USER_MAX) {
+ /*
+ * Fill the cache upon first access, since
+ * when func is created, no user may
+ * be around to fill it (recovery of
+ * system spaces from a snapshot).
+ */
+ struct user *owner = user_find_xc(func->def.uid);
+ credentials_init(&func->owner_credentials, owner);
+ }
+ fiber_set_user(fiber(), &func->owner_credentials);
+ }
+
+ int rc;
+ if (func != NULL && func->def.language == FUNC_LANGUAGE_C) {
+ rc = func_call(func, request, out);
+ } else {
+ rc = box_lua_call(func, request, out);
+ }
+
+ /*
+ * Restore original user
+ */
+ if (orig_credentials != NULL)
+ fiber_set_user(fiber(), orig_credentials);
+
+ if (rc != 0) {
+ txn_rollback();
+ diag_raise();
+ }
+
+ if (in_txn()) {
+ /* The procedure forgot to call box.commit() */
+ say_warn("a transaction is active at CALL return from '%.*s'",
+ name_len, name);
+ txn_rollback();
+ }
+}
+
void
box_process_join(int fd, struct xrow_header *header)
{
diff --git a/src/box/box.h b/src/box/box.h
index e948aea6ee..1b7571eaa9 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -47,6 +47,7 @@ extern "C" {
struct port;
struct request;
struct xrow_header;
+struct obuf;
/** To be called at program start. */
void box_load_cfg();
@@ -83,6 +84,9 @@ const char *box_status(void);
void
box_process_auth(struct request *request);
+void
+box_process_call(struct request *request, struct obuf *out);
+
void
box_process_join(int fd, struct xrow_header *header);
diff --git a/src/box/func.cc b/src/box/func.cc
index a78f9e3b74..db62c1fd74 100644
--- a/src/box/func.cc
+++ b/src/box/func.cc
@@ -33,8 +33,14 @@
#include <dlfcn.h>
#include "lua/utils.h"
+#include "diag.h"
#include "scoped_guard.h"
+#include "box/request.h"
+#include "box/txn.h"
+#include "box/port.h"
+#include "box/iproto_port.h"
+
struct func *
func_new(struct func_def *def)
{
@@ -142,3 +148,54 @@ func_delete(struct func *func)
func_unload(func);
free(func);
}
+
+int
+func_call(struct func *func, struct request *request, struct obuf *out)
+{
+ assert(func != NULL && func->def.language == FUNC_LANGUAGE_C);
+ if (func->func == NULL)
+ func_load(func);
+
+ /* Create a call context */
+ struct port_buf port_buf;
+ port_buf_create(&port_buf);
+ box_function_ctx_t ctx = { request, &port_buf.base };
+
+ /* Clear all previous errors */
+ diag_clear(&fiber()->diag);
+ assert(!in_txn()); /* transaction is not started */
+ /* Call function from the shared library */
+ int rc = func->func(&ctx, request->tuple, request->tuple_end);
+ if (rc != 0) {
+ if (diag_last_error(&fiber()->diag) == NULL) {
+ /* Stored procedure forget to set diag */
+ diag_set(ClientError, ER_PROC_C,
+ "unknown procedure error");
+ }
+ goto error;
+ }
+
+ /* Push results to obuf */
+ struct obuf_svp svp;
+ if (iproto_prepare_select(out, &svp) != 0)
+ goto error;
+
+ for (struct port_buf_entry *entry = port_buf.first;
+ entry != NULL; entry = entry->next) {
+ if (tuple_to_obuf(entry->tuple, out) != 0) {
+ obuf_rollback_to_svp(out, &svp);
+ goto error;
+ }
+ }
+ iproto_reply_select(out, &svp, request->header->sync,
+ port_buf.size);
+
+ port_buf_destroy(&port_buf);
+
+ return 0;
+
+error:
+ port_buf_destroy(&port_buf);
+ txn_rollback();
+ return -1;
+}
diff --git a/src/box/func.h b/src/box/func.h
index 99bc9d49fe..f7c0f9e12c 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -33,6 +33,10 @@
#include "key_def.h"
#include "request.h"
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
/**
* Stored function.
*/
@@ -58,6 +62,15 @@ struct func {
struct access access[BOX_USER_MAX];
};
+struct request;
+struct obuf;
+
+int
+func_call(struct func *func, struct request *request, struct obuf *out);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+
struct func *
func_new(struct func_def *def);
@@ -74,4 +87,6 @@ func_delete(struct func *func);
void
func_load(struct func *func);
+#endif /* defined(__cplusplus) */
+
#endif /* TARANTOOL_BOX_FUNC_H_INCLUDED */
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index b285068b6b..6766abbad9 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -686,9 +686,13 @@ tx_process_msg(struct cmsg *m)
struct tuple *tuple;
if (box_process1(&msg->request, &tuple) < 0)
diag_raise();
- struct obuf_svp svp = iproto_prepare_select(out);
- if (tuple)
- tuple_to_obuf(tuple, out);
+ struct obuf_svp svp;
+ if (iproto_prepare_select(out, &svp) != 0)
+ diag_raise();
+ if (tuple) {
+ if (tuple_to_obuf(tuple, out) != 0)
+ diag_raise();
+ }
iproto_reply_select(out, &svp, msg->header.sync,
tuple != 0);
break;
@@ -696,7 +700,7 @@ tx_process_msg(struct cmsg *m)
case IPROTO_CALL:
assert(msg->request.type == msg->header.type);
rmean_collect(rmean_box, msg->request.type, 1);
- box_lua_call(&msg->request, out);
+ box_process_call(&msg->request, out);
break;
case IPROTO_EVAL:
assert(msg->request.type == msg->header.type);
diff --git a/src/box/iproto_port.cc b/src/box/iproto_port.cc
index 0d8a6621ab..3544483703 100644
--- a/src/box/iproto_port.cc
+++ b/src/box/iproto_port.cc
@@ -126,16 +126,25 @@ iproto_port_eof(struct port *ptr)
struct iproto_port *port = iproto_port(ptr);
/* found == 0 means add_tuple wasn't called at all. */
if (port->found == 0) {
- port->svp = iproto_prepare_select(port->buf);
+ if (iproto_prepare_select(port->buf, &port->svp) != 0)
+ diag_raise();
}
iproto_reply_select(port->buf, &port->svp, port->sync, port->found);
}
-struct obuf_svp
-iproto_prepare_select(struct obuf *buf)
+int
+iproto_prepare_select(struct obuf *buf, struct obuf_svp *svp)
{
- return obuf_book_xc(buf, SVP_SIZE);
+ void *ptr = obuf_reserve(buf, SVP_SIZE);
+ if (ptr == NULL) {
+ diag_set(OutOfMemory, SVP_SIZE, "obuf", "reserve");
+ return -1;
+ }
+ *svp = obuf_create_svp(buf);
+ ptr = obuf_alloc(buf, SVP_SIZE);
+ assert(ptr != NULL);
+ return 0;
}
void
@@ -163,7 +172,8 @@ iproto_port_add_tuple(struct port *ptr, struct tuple *tuple)
struct iproto_port *port = iproto_port(ptr);
if (port->found == 0) {
/* Found the first tuple, add header. */
- port->svp = iproto_prepare_select(port->buf);
+ if (iproto_prepare_select(port->buf, &port->svp) != 0)
+ diag_raise();
}
port->found++;
tuple_to_obuf(tuple, port->buf);
diff --git a/src/box/iproto_port.h b/src/box/iproto_port.h
index ab6aef3b12..e0ac554236 100644
--- a/src/box/iproto_port.h
+++ b/src/box/iproto_port.h
@@ -88,8 +88,8 @@ iproto_reply_ok(struct obuf *out, uint64_t sync);
void
iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync);
-struct obuf_svp
-iproto_prepare_select(struct obuf *buf);
+int
+iproto_prepare_select(struct obuf *buf, struct obuf_svp *svp);
void
iproto_reply_select(struct obuf *buf, struct obuf_svp *svp, uint64_t sync,
diff --git a/src/box/lua/call.cc b/src/box/lua/call.cc
index a5002a99a9..f4d890ea72 100644
--- a/src/box/lua/call.cc
+++ b/src/box/lua/call.cc
@@ -128,128 +128,6 @@ lbox_call_loadproc(struct lua_State *L)
return box_lua_find(L, name, name + name_len);
}
-/**
- * Check access to a function and change the current
- * user id if the function is a set-definer-user-id one.
- * The original user is restored in the destructor.
- */
-struct SetuidGuard
-{
- /** True if the function was set-user-id one. */
- bool setuid;
- struct credentials *orig_credentials;
-
- inline SetuidGuard(const char *name, uint32_t name_len,
- uint8_t access, struct func *func);
- inline ~SetuidGuard();
-};
-
-SetuidGuard::SetuidGuard(const char *name, uint32_t name_len,
- uint8_t access, struct func *func)
- :setuid(false)
- ,orig_credentials(current_user())
-{
-
- /*
- * If the user has universal access, don't bother with setuid.
- * No special check for ADMIN user is necessary
- * since ADMIN has universal access.
- */
- if ((orig_credentials->universal_access & PRIV_ALL) == PRIV_ALL)
- return;
- access &= ~orig_credentials->universal_access;
- if (func == NULL && access == 0) {
- /**
- * Well, the function is not explicitly defined,
- * so it's obviously not a setuid one.
- */
- return;
- }
- if (func == NULL || (func->def.uid != orig_credentials->uid &&
- access & ~func->access[orig_credentials->auth_token].effective)) {
- /* Access violation, report error. */
- char name_buf[BOX_NAME_MAX + 1];
- snprintf(name_buf, sizeof(name_buf), "%.*s", name_len, name);
- struct user *user = user_find_xc(orig_credentials->uid);
-
- tnt_raise(ClientError, ER_FUNCTION_ACCESS_DENIED,
- priv_name(access), user->def.name, name_buf);
- }
- if (func->def.setuid) {
- /** Remember and change the current user id. */
- if (func->owner_credentials.auth_token >= BOX_USER_MAX) {
- /*
- * Fill the cache upon first access, since
- * when func is created, no user may
- * be around to fill it (recovery of
- * system spaces from a snapshot).
- */
- struct user *owner = user_find_xc(func->def.uid);
- credentials_init(&func->owner_credentials, owner);
- }
- setuid = true;
- fiber_set_user(fiber(), &func->owner_credentials);
- }
-}
-
-SetuidGuard::~SetuidGuard()
-{
- if (setuid)
- fiber_set_user(fiber(), orig_credentials);
-}
-
-
-static inline void
-execute_c_call(struct func *func, struct request *request, struct obuf *out)
-{
- assert(func != NULL && func->def.language == FUNC_LANGUAGE_C);
- if (func->func == NULL)
- func_load(func);
-
- const char *name = request->key;
- uint32_t name_len = mp_decode_strl(&name);
- SetuidGuard setuid(name, name_len, PRIV_X, func);
-
- struct port_buf port_buf;
- port_buf_create(&port_buf);
- auto guard = make_scoped_guard([&]{
- port_buf_destroy(&port_buf);
- });
-
- box_function_ctx_t ctx = { request, &port_buf.base };
- int rc = 0;
- try {
- rc = func->func(&ctx, request->tuple, request->tuple_end);
- } catch (...) {
- panic("C++ exception thrown from stored C function");
- }
- if (in_txn()) {
- say_warn("a transaction is active at CALL return from '%.*s'",
- name_len, name);
- txn_rollback();
- }
-
- if (rc != 0) {
- diag_raise();
- tnt_raise(ClientError, ER_PROC_C, "unknown procedure error");
- }
-
- struct obuf_svp svp = iproto_prepare_select(out);
- try {
- for (struct port_buf_entry *entry = port_buf.first;
- entry != NULL; entry = entry->next) {
- tuple_to_obuf(entry->tuple, out);
- }
- iproto_reply_select(out, &svp, request->header->sync,
- port_buf.size);
- } catch (Exception *e) {
- obuf_rollback_to_svp(out, &svp);
- txn_rollback();
- /* Let all well-behaved exceptions pass through. */
- throw;
- }
-}
-
static inline void
luamp_encode_tuple_xc(struct lua_State *L, struct luaL_serializer *cfg,
struct mpstream *stream, int index)
@@ -275,8 +153,7 @@ luamp_convert_tuple_xc(struct lua_State *L, struct luaL_serializer *cfg,
* (implementation of 'CALL' command code).
*/
static inline void
-execute_lua_call(lua_State *L, struct func *func, struct request *request,
- struct obuf *out)
+execute_lua_call(lua_State *L, struct request *request, struct obuf *out)
{
const char *name = request->key;
uint32_t name_len = mp_decode_strl(&name);
@@ -284,14 +161,7 @@ execute_lua_call(lua_State *L, struct func *func, struct request *request,
int oc = 0; /* how many objects are on stack after box_lua_find */
/* Try to find a function by name in Lua */
oc = box_lua_find(L, name, name + name_len);
- /**
- * Check access to the function and optionally change
- * execution time user id (set user id). Sic: the order
- * is important, as is described in
- * https://github.com/tarantool/tarantool/issues/300
- * - if a function does not exist, say it first.
- */
- SetuidGuard setuid(name, name_len, PRIV_X, func);
+
/* Push the rest of args (a tuple). */
const char *args = request->tuple;
@@ -302,12 +172,6 @@ execute_lua_call(lua_State *L, struct func *func, struct request *request,
luamp_decode(L, luaL_msgpack_default, &args);
lbox_call_xc(L, arg_count + oc - 1, LUA_MULTRET);
- if (in_txn()) {
- say_warn("a transaction is active at CALL return from '%.*s'",
- name_len, name);
- txn_rollback();
- }
-
/**
* Add all elements from Lua stack to iproto.
*
@@ -328,7 +192,9 @@ execute_lua_call(lua_State *L, struct func *func, struct request *request,
* while Lua table size is pretty much unlimited.
*/
uint32_t count = 0;
- struct obuf_svp svp = iproto_prepare_select(out);
+ struct obuf_svp svp;
+ if (iproto_prepare_select(out, &svp) != 0)
+ diag_raise();
struct mpstream stream;
mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
luamp_error, L);
@@ -370,34 +236,28 @@ execute_lua_call(lua_State *L, struct func *func, struct request *request,
}
}
-void
-box_lua_call(struct request *request, struct obuf *out)
+int
+box_lua_call(struct func *func, struct request *request, struct obuf *out)
{
- const char *name = request->key;
- uint32_t name_len = mp_decode_strl(&name);
-
- struct func *func = func_by_name(name, name_len);
- if (func != NULL && func->def.language == FUNC_LANGUAGE_C)
- return execute_c_call(func, request, out);
-
/*
* func == NULL means that perhaps the user has a global
* "EXECUTE" privilege, so no specific grant to a function.
*/
assert(func == NULL || func->def.language == FUNC_LANGUAGE_LUA);
+ (void) func;
lua_State *L = NULL;
try {
L = lua_newthread(tarantool_L);
LuarefGuard coro_ref(tarantool_L);
- execute_lua_call(L, func, request, out);
+ execute_lua_call(L, request, out);
+ return 0;
} catch (Exception *e) {
- txn_rollback();
/* Let all well-behaved exceptions pass through. */
- throw;
+ return -1;
} catch (...) {
- txn_rollback();
/* Convert Lua error to a Tarantool exception. */
- tnt_raise(LuajitError, lua_tostring(L ? L : tarantool_L, -1));
+ diag_set(LuajitError, lua_tostring(L ? L : tarantool_L, -1));
+ return -1;
}
}
@@ -425,7 +285,9 @@ execute_eval(lua_State *L, struct request *request, struct obuf *out)
lbox_call_xc(L, arg_count, LUA_MULTRET);
/* Send results of the called procedure to the client. */
- struct obuf_svp svp = iproto_prepare_select(out);
+ struct obuf_svp svp;
+ if (iproto_prepare_select(out, &svp) != 0)
+ diag_raise();
struct mpstream stream;
mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
luamp_error, L);
diff --git a/src/box/lua/call.h b/src/box/lua/call.h
index e3f8a0db4f..cc264c774f 100644
--- a/src/box/lua/call.h
+++ b/src/box/lua/call.h
@@ -53,8 +53,8 @@ struct obuf;
* Invoke a Lua stored procedure from the binary protocol
* (implementation of 'CALL' command code).
*/
-void
-box_lua_call(struct request *request, struct obuf *out);
+int
+box_lua_call(struct func *func, struct request *request, struct obuf *out);
void
box_lua_eval(struct request *request, struct obuf *out);
diff --git a/src/box/tuple.h b/src/box/tuple.h
index bcf11512a8..aa295a354d 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -809,7 +809,7 @@ tuple_compare_with_key(const struct tuple *tuple_a, const char *key,
/** These functions are implemented in tuple_convert.cc. */
/* Store tuple in the output buffer in iproto format. */
-void
+int
tuple_to_obuf(struct tuple *tuple, struct obuf *buf);
/**
diff --git a/src/box/tuple_convert.cc b/src/box/tuple_convert.cc
index 75b782f2af..e859c2aa53 100644
--- a/src/box/tuple_convert.cc
+++ b/src/box/tuple_convert.cc
@@ -31,10 +31,14 @@
#include "tuple.h"
#include "iobuf.h"
-void
+int
tuple_to_obuf(struct tuple *tuple, struct obuf *buf)
{
- obuf_dup_xc(buf, tuple->data, tuple->bsize);
+ if (obuf_dup(buf, tuple->data, tuple->bsize) != tuple->bsize) {
+ diag_set(OutOfMemory, tuple->bsize, "tuple_to_obuf", "dup");
+ return -1;
+ }
+ return 0;
}
ssize_t
diff --git a/test/box/access.result b/test/box/access.result
index 62f170a696..e85eeaa5a4 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -231,7 +231,7 @@ c = (require 'net.box'):new(LISTEN.host, LISTEN.service)
...
c:call('nosuchfunction')
---
-- error: Procedure 'nosuchfunction' is not defined
+- error: Execute access denied for user 'guest' to function 'nosuchfunction'
...
function nosuchfunction() end
---
@@ -245,7 +245,7 @@ nosuchfunction = nil
...
c:call('nosuchfunction')
---
-- error: Procedure 'nosuchfunction' is not defined
+- error: Execute access denied for user 'guest' to function 'nosuchfunction'
...
c:close()
---
diff --git a/test/box/net.box.result b/test/box/net.box.result
index f3e7ada07f..b8aee958d9 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -60,7 +60,7 @@ cn:ping()
-- check permissions
cn:call('unexists_procedure')
---
-- error: Procedure 'unexists_procedure' is not defined
+- error: Execute access denied for user 'guest' to function 'unexists_procedure'
...
function test_foo(a,b,c) return { {{ [a] = 1 }}, {{ [b] = 2 }}, c } end
---
@@ -84,15 +84,15 @@ cn = remote:new(box.cfg.listen)
...
cn:call('unexists_procedure')
---
-- error: Procedure 'unexists_procedure' is not defined
+- error: Execute access denied for user 'guest' to function 'unexists_procedure'
...
cn:call('test_foo', 'a', 'b', 'c')
---
-- error: Tuple/Key must be MsgPack array
+- error: Execute access denied for user 'guest' to function 'test_foo'
...
cn:call(nil, 'a', 'b', 'c')
---
-- error: Procedure 'nil' is not defined
+- error: Execute access denied for user 'guest' to function 'nil'
...
cn:eval('return 2+2')
---
--
GitLab