jenkins-bot has submitted this change and it was merged.
Change subject: Invalid data should cause an error, not a silent placeholder
......................................................................
Invalid data should cause an error, not a silent placeholder
The behavior is inconsistent between LuaStandalone and LuaSandbox: in
the former, circular data structures and unsupported types cause an
error, while in the latter they are silently replaced with a
"placeholder" object.
The former behavior seems more logical, so let's do that.
Bug: 48393
Change-Id: I99c80549aebbd2ae1b7fbf8ab11f8588b40855fd
---
M data_conversion.c
M luasandbox.c
M php_luasandbox.h
M tests/call.phpt
4 files changed, 135 insertions(+), 49 deletions(-)
Approvals:
Tim Starling: Looks good to me, approved
jenkins-bot: Verified
diff --git a/data_conversion.c b/data_conversion.c
index 85f62df..be7af4e 100644
--- a/data_conversion.c
+++ b/data_conversion.c
@@ -11,15 +11,16 @@
#include "php.h"
#include "php_luasandbox.h"
+#include "zend_exceptions.h"
-static void luasandbox_lua_to_array(HashTable *ht, lua_State *L, int index,
+static int luasandbox_lua_to_array(HashTable *ht, lua_State *L, int index,
zval * sandbox_zval, HashTable * recursionGuard TSRMLS_DC);
static int luasandbox_free_zval_userdata(lua_State * L);
static int luasandbox_push_hashtable(lua_State * L, HashTable * ht);
static int luasandbox_has_error_marker(lua_State * L, int index, void *
marker);
extern zend_class_entry *luasandboxfunction_ce;
-extern zend_class_entry *luasandboxplaceholder_ce;
+extern zend_class_entry *luasandboxruntimeerror_ce;
/**
* An int, the address of which is used as a fatal error marker. The value is
@@ -193,8 +194,8 @@
*
* Convert a lua value to a zval.
*
- * If a value is encountered that can't be converted to a zval, a
LuaPlaceholder
- * object is returned instead.
+ * If a value is encountered that can't be converted to a zval, an exception is
+ * thrown.
*
* @param z A pointer to the destination zval
* @param L The lua state
@@ -204,8 +205,9 @@
* @param recursionGuard A hashtable for keeping track of tables that have
been
* processed, to allow infinite recursion to be avoided. External callers
* should set this to NULL.
+ * @return int 0 (and a PHP exception) on failure
*/
-void luasandbox_lua_to_zval(zval * z, lua_State * L, int index,
+int luasandbox_lua_to_zval(zval * z, lua_State * L, int index,
zval * sandbox_zval, HashTable * recursionGuard TSRMLS_DC)
{
switch (lua_type(L, index)) {
@@ -251,12 +253,29 @@
const void * ptr = lua_topointer(L, index);
void * data = NULL;
int allocated = 0;
+ int success = 1;
if (recursionGuard) {
// Check for circular reference (infinite
recursion)
if (zend_hash_find(recursionGuard, (char*)&ptr,
sizeof(void*), &data) == SUCCESS) {
// Found circular reference!
- object_init_ex(z,
luasandboxplaceholder_ce);
- break;
+ zval *zex, *ztrace;
+ MAKE_STD_ZVAL(zex);
+ object_init_ex(zex,
luasandboxruntimeerror_ce);
+
+ MAKE_STD_ZVAL(ztrace);
+ luasandbox_push_structured_trace(L, 1);
+ luasandbox_lua_to_zval(ztrace, L, -1,
sandbox_zval, NULL TSRMLS_CC);
+
zend_update_property(luasandboxruntimeerror_ce, zex, "luaTrace",
sizeof("luaTrace")-1, ztrace TSRMLS_CC);
+ zval_ptr_dtor(&ztrace);
+ lua_pop(L, 1);
+
+
zend_update_property_string(luasandboxruntimeerror_ce, zex,
+ "message", sizeof("message")-1,
"Cannot pass circular reference to PHP" TSRMLS_CC);
+
zend_update_property_long(luasandboxruntimeerror_ce, zex, "code",
sizeof("code")-1, -1 TSRMLS_CC);
+ zend_throw_exception_object(zex
TSRMLS_CC);
+
+ ZVAL_NULL(z); // Need to set something
to prevent a segfault
+ return 0;
}
} else {
ALLOC_HASHTABLE(recursionGuard);
@@ -270,11 +289,15 @@
// Process the array
array_init(z);
- luasandbox_lua_to_array(Z_ARRVAL_P(z), L, index,
sandbox_zval, recursionGuard TSRMLS_CC);
+ success = luasandbox_lua_to_array(Z_ARRVAL_P(z), L,
index, sandbox_zval, recursionGuard TSRMLS_CC);
if (allocated) {
zend_hash_destroy(recursionGuard);
FREE_HASHTABLE(recursionGuard);
+ }
+
+ if (!success) {
+ return 0;
}
break;
}
@@ -318,10 +341,35 @@
case LUA_TUSERDATA:
case LUA_TTHREAD:
case LUA_TLIGHTUSERDATA:
- default:
// TODO: provide derived classes for each type
- object_init_ex(z, luasandboxplaceholder_ce);
+ default: {
+ zval *zex, *ztrace;
+ char *message;
+
+ spprintf(&message, 0, "Cannot pass %s to PHP",
lua_typename(L, lua_type(L, index)));
+
+ MAKE_STD_ZVAL(zex);
+ object_init_ex(zex, luasandboxruntimeerror_ce);
+
+ MAKE_STD_ZVAL(ztrace);
+ luasandbox_push_structured_trace(L, 1);
+ luasandbox_lua_to_zval(ztrace, L, -1, sandbox_zval,
NULL TSRMLS_CC);
+ zend_update_property(luasandboxruntimeerror_ce, zex,
"luaTrace", sizeof("luaTrace")-1, ztrace TSRMLS_CC);
+ zval_ptr_dtor(&ztrace);
+ lua_pop(L, 1);
+
+ zend_update_property_string(luasandboxruntimeerror_ce,
zex,
+ "message", sizeof("message")-1, message
TSRMLS_CC);
+ zend_update_property_long(luasandboxruntimeerror_ce,
zex, "code", sizeof("code")-1, -1 TSRMLS_CC);
+ zend_throw_exception_object(zex TSRMLS_CC);
+
+ efree(message);
+
+ ZVAL_NULL(z); // Need to set something to prevent a
segfault
+ return 0;
+ }
}
+ return 1;
}
/* }}} */
@@ -329,7 +377,7 @@
*
* Append the elements of the table in the specified index to the given
HashTable.
*/
-static void luasandbox_lua_to_array(HashTable *ht, lua_State *L, int index,
+static int luasandbox_lua_to_array(HashTable *ht, lua_State *L, int index,
zval * sandbox_zval, HashTable * recursionGuard TSRMLS_DC)
{
const char * str;
@@ -346,8 +394,12 @@
lua_pushnil(L);
while (lua_next(L, index) != 0) {
MAKE_STD_ZVAL(value);
- luasandbox_lua_to_zval(value, L, -1, sandbox_zval,
recursionGuard TSRMLS_CC);
-
+ if (!luasandbox_lua_to_zval(value, L, -1, sandbox_zval,
recursionGuard TSRMLS_CC)) {
+ // Conversion failed, fix stack and bail
+ lua_settop(L, top);
+ return 0;
+ }
+
if (lua_type(L, -2) == LUA_TNUMBER) {
n = lua_tonumber(L, -2);
if (n == floor(n)) {
@@ -366,6 +418,7 @@
// Pop temporary values off the stack
lua_settop(L, top + 1);
}
+ return 1;
}
/* }}} */
diff --git a/luasandbox.c b/luasandbox.c
index 936b3b0..d353f06 100644
--- a/luasandbox.c
+++ b/luasandbox.c
@@ -72,7 +72,6 @@
zend_class_entry *luasandboxerrorerror_ce;
zend_class_entry *luasandboxtimeouterror_ce;
zend_class_entry *luasandboxemergencytimeouterror_ce;
-zend_class_entry *luasandboxplaceholder_ce;
zend_class_entry *luasandboxfunction_ce;
ZEND_DECLARE_MODULE_GLOBALS(luasandbox);
@@ -276,9 +275,6 @@
INIT_CLASS_ENTRY(ce, "LuaSandboxEmergencyTimeoutError",
luasandbox_empty_methods);
luasandboxemergencytimeouterror_ce = zend_register_internal_class_ex(
&ce, luasandboxfatalerror_ce, NULL TSRMLS_CC);
-
- INIT_CLASS_ENTRY(ce, "LuaSandboxPlaceholder", luasandbox_empty_methods);
- luasandboxplaceholder_ce = zend_register_internal_class(&ce TSRMLS_CC);
INIT_CLASS_ENTRY(ce, "LuaSandboxFunction", luasandboxfunction_methods);
luasandboxfunction_ce = zend_register_internal_class(&ce TSRMLS_CC);
@@ -598,8 +594,9 @@
}
// Make a zval out of it, and return false on error
- luasandbox_lua_to_zval(return_value, L, lua_gettop(L), this_ptr, NULL
TSRMLS_CC);
- if (Z_TYPE_P(return_value) == IS_NULL) {
+ if (!luasandbox_lua_to_zval(return_value, L, lua_gettop(L), this_ptr,
NULL TSRMLS_CC) ||
+ Z_TYPE_P(return_value) == IS_NULL
+ ) {
php_error_docref(NULL TSRMLS_CC, E_WARNING,
"too many chunks loaded already");
RETVAL_FALSE;
@@ -1169,8 +1166,9 @@
luasandbox_push_zval_userdata(L, z);
lua_pushcclosure(L, luasandbox_call_php, 1);
- luasandbox_lua_to_zval(return_value, L, lua_gettop(L), this_ptr, NULL
TSRMLS_CC);
- if (Z_TYPE_P(return_value) == IS_NULL) {
+ if (!luasandbox_lua_to_zval(return_value, L, lua_gettop(L), this_ptr,
NULL TSRMLS_CC) ||
+ Z_TYPE_P(return_value) == IS_NULL
+ ) {
php_error_docref(NULL TSRMLS_CC, E_WARNING,
"too many chunks loaded already");
RETVAL_FALSE;
@@ -1378,7 +1376,10 @@
for (i = 0; i < numResults; i++) {
zval * element;
MAKE_STD_ZVAL(element);
- luasandbox_lua_to_zval(element, L, retIndex + i, sandbox_zval,
NULL TSRMLS_CC);
+ if (!luasandbox_lua_to_zval(element, L, retIndex + i,
sandbox_zval, NULL TSRMLS_CC)) {
+ // Convert failed (which means an exception), so bail.
+ break;
+ }
zend_hash_next_index_insert(Z_ARRVAL_P(return_value),
(void*)&element,
sizeof(zval*), NULL);
@@ -1585,46 +1586,55 @@
fci.retval_ptr_ptr = &retval_ptr;
// Make an array of zval double-pointers to hold the arguments
+ int args_failed = 0;
temp = ecalloc(top, sizeof(void*) * 2);
double_pointers = (zval***)temp;
pointers = (zval**)(temp + top);
for (i = 0; i < top; i++ ) {
MAKE_STD_ZVAL(pointers[i]);
- luasandbox_lua_to_zval(pointers[i], L, i + 1,
intern->current_zval, NULL TSRMLS_CC);
+ if (!luasandbox_lua_to_zval(pointers[i], L, i + 1,
intern->current_zval, NULL TSRMLS_CC)) {
+ // Argument conversion failed, so skip the call. The
PHP exception
+ // from the conversion will be handled below.
+ args_failed = 1;
+ top = i + 1;
+ break;
+ }
double_pointers[i] = &(pointers[i]);
}
- // Initialise the fci. Use zend_fcall_info_args_restore() since that's
an
- // almost-legitimate way to avoid the extra malloc that we'd get from
- // zend_fcall_info_argp()
- zend_fcall_info_args_restore(&fci, top, double_pointers);
+ if (!args_failed) {
+ // Initialise the fci. Use zend_fcall_info_args_restore() since
that's an
+ // almost-legitimate way to avoid the extra malloc that we'd
get from
+ // zend_fcall_info_argp()
+ zend_fcall_info_args_restore(&fci, top, double_pointers);
- // Sanity check, timers should never be paused at this point
- assert(!luasandbox_timer_is_paused(&intern->timer));
+ // Sanity check, timers should never be paused at this point
+ assert(!luasandbox_timer_is_paused(&intern->timer));
- // Call the function
- status = zend_call_function(&fci, &fcc TSRMLS_CC);
+ // Call the function
+ status = zend_call_function(&fci, &fcc TSRMLS_CC);
- // Automatically unpause now that PHP has returned
- luasandbox_timer_unpause(&intern->timer);
+ // Automatically unpause now that PHP has returned
+ luasandbox_timer_unpause(&intern->timer);
- if (status == SUCCESS && fci.retval_ptr_ptr && *fci.retval_ptr_ptr)
- {
- // Push the return values back to Lua
- if (Z_TYPE_PP(fci.retval_ptr_ptr) == IS_NULL) {
- // No action
- } else if (Z_TYPE_PP(fci.retval_ptr_ptr) == IS_ARRAY) {
- bucket = Z_ARRVAL_PP(fci.retval_ptr_ptr)->pListHead;
- while (bucket) {
- luasandbox_push_zval(L, *((zval
**)bucket->pData));
- bucket = bucket->pListNext;
- num_results++;
+ if (status == SUCCESS && fci.retval_ptr_ptr &&
*fci.retval_ptr_ptr)
+ {
+ // Push the return values back to Lua
+ if (Z_TYPE_PP(fci.retval_ptr_ptr) == IS_NULL) {
+ // No action
+ } else if (Z_TYPE_PP(fci.retval_ptr_ptr) == IS_ARRAY) {
+ bucket =
Z_ARRVAL_PP(fci.retval_ptr_ptr)->pListHead;
+ while (bucket) {
+ luasandbox_push_zval(L, *((zval
**)bucket->pData));
+ bucket = bucket->pListNext;
+ num_results++;
+ }
+ } else {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING,
+ "function tried to return a single
value to Lua without wrapping it in an array");
}
- } else {
- php_error_docref(NULL TSRMLS_CC, E_WARNING,
- "function tried to return a single value to Lua
without wrapping it in an array");
+ zval_ptr_dtor(&retval_ptr);
}
- zval_ptr_dtor(&retval_ptr);
}
// Free the argument zvals
diff --git a/php_luasandbox.h b/php_luasandbox.h
index 82b6bdf..5289126 100644
--- a/php_luasandbox.h
+++ b/php_luasandbox.h
@@ -129,7 +129,7 @@
int luasandbox_push_zval(lua_State * L, zval * z);
void luasandbox_push_zval_userdata(lua_State * L, zval * z);
-void luasandbox_lua_to_zval(zval * z, lua_State * L, int index,
+int luasandbox_lua_to_zval(zval * z, lua_State * L, int index,
zval * sandbox_zval, HashTable * recursionGuard TSRMLS_DC);
void luasandbox_wrap_fatal(lua_State * L);
int luasandbox_is_fatal(lua_State * L, int index);
diff --git a/tests/call.phpt b/tests/call.phpt
index bc1e97a..9fb1ff8 100644
--- a/tests/call.phpt
+++ b/tests/call.phpt
@@ -4,8 +4,31 @@
<?php
$sandbox = new LuaSandbox;
var_dump( $sandbox->loadString( 'return 1' )->call() );
+
+echo "Proper handling of circular tables returned by Lua: ";
+$sandbox = new LuaSandbox;
+try {
+ $ret = $sandbox->loadString( 'local t = {}; t.t = t; return t'
)->call();
+ echo var_export( $ret, 1 ) . "\n";
+} catch ( Exception $ex ) {
+ echo "Exception: " . $ex->getMessage() . "\n";
+}
+
+echo "Proper handling of circular tables in Lua→PHP call: ";
+$sandbox = new LuaSandbox;
+$f = $sandbox->wrapPhpFunction( function () {
+ echo func_num_args() . " args ok\n";
+} );
+try {
+ $sandbox->loadString( 'local f = ...; local t = {}; t.t = t; f( t )'
)->call( $f );
+} catch ( Exception $ex ) {
+ echo "Exception: " . $ex->getMessage() . "\n";
+}
+
--EXPECT--
array(1) {
[0]=>
int(1)
}
+Proper handling of circular tables returned by Lua: Exception: Cannot pass
circular reference to PHP
+Proper handling of circular tables in Lua→PHP call: Exception: Cannot pass
circular reference to PHP
--
To view, visit https://gerrit.wikimedia.org/r/63565
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I99c80549aebbd2ae1b7fbf8ab11f8588b40855fd
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/php/luasandbox
Gerrit-Branch: master
Gerrit-Owner: Anomie <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Anomie <[email protected]>
Gerrit-Reviewer: Tim Starling <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
