8000 Fix OASIS-24962 (#17956) · arangodb/arangodb@b1f1325 · GitHub
[go: up one dir, main page]
Skip to content

Commit b1f1325

Browse files
jsteemannKVS85
jsteemann
and
KVS85
authored
Fix OASIS-24962 (#17956)
* Fix OASIS-24962 * Fix coordinator segfault in AQL queries in which the query is invoked from within a JavaScript context (e.g. from Foxx or from the server's console mode) **and** the query has multiple coordinator snippets of which except the outermost one invokes a JavaScript function. Instead of crashing, coordinators will now respond with the exception "no v8 context available to enter for current transaction context". For AQL queries that called one of the AQL functions `CALL` or `APPLY` with a fixed function name, e.g. `APPLY('CONCAT', ...)`, it is now also assumed correctly that no JavaScript is needed, except if the fixed function name is the name of a user-defined function. This fixes an issue described in OASIS-24962. * fix failing tests * fix crash in test * fix failing assertion * fix yet another assertion * Update arangod/Aql/Function.cpp Co-authored-by: Vadim <vadim@arangodb.com>
1 parent 78a1e38 commit b1f1325

File tree

11 files changed

+220
-15
lines changed

11 files changed

+220
-15
lines changed

‎CHANGELOG

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,18 @@
11
v3.8.9 (XXXX-XX-XX)
22
-------------------
33

4+
* Fix coordinator segfault in AQL queries in which the query is invoked from
5+
within a JavaScript context (e.g. from Foxx or from the server's console mode)
6+
**and** the query has multiple coordinator snippets of which except the
7+
outermost one invokes a JavaScript function.
8+
Instead of crashing, coordinators will now respond with the exception "no v8
9+
context available to enter for current transaction context".
10+
For AQL queries that called one of the AQL functions `CALL` or `APPLY` with a
11+
fixed function name, e.g. `APPLY('CONCAT', ...)`, it is now also assumed
12+
correctly that no JavaScript is needed, except if the fixed function name is
13+
the name of a user-defined function.
14+
This fixes an issue described in OASIS-24962.
15+
416
* Prevent agency configuration confusion by an agent which comes back without
517
its data directory and thus without its UUID.
618

‎arangod/Aql/Ast.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -572,10 +572,11 @@ class Ast {
572572
/// @brief traverse the AST using a depth-first visitor, with const nodes
573573
static void traverseReadOnly(AstNode const*, std::function<void(AstNode const*)> const&);
574574

575-
private:
576575
/// @brief normalize a function name
577-
std::pair<std::string, bool> normalizeFunctionName(char const* functionName, size_t length);
576+
static std::pair<std::string, bool> normalizeFunctionName(
577+
char const* functionName, size_t length);
578578

579+
private:
579580
/// @brief create a node of the specified type
580581
AstNode* createNode(AstNodeType);
581582

‎arangod/Aql/AstNode.cpp

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1666,15 +1666,33 @@ bool AstNode::willUseV8() const {
16661666

16671667
if (func->name == "CALL" || func->name == "APPLY") {
16681668
// CALL and APPLY can call arbitrary other functions...
1669-
if (numMembers() > 0 && getMemberUnchecked(0)->isStringValue()) {
1670-
auto s = getMemberUnchecked(0)->getStringRef();
1671-
if (s.find(':') != std::string::npos) {
1669+
if (numMembers() > 0 && getMemberUnchecked(0)->isArray() &&
1670+
getMemberUnchecked(0)->numMembers() > 0 &&
1671+
getMemberUnchecked(0)->getMemberUnchecked(0)->isStringValue()) {
1672+
// calling a function with a fixed name (known an query compile time)
1673+
if (auto s =
1674+
getMemberUnchecked(0)->getMemberUnchecked(0)->getStringRef();
1675+
s.find(':') != std::string::npos) {
16721676
// a user-defined function.
16731677
// this will use V8
16741678
setFlag(DETERMINED_V8, VALUE_V8);
16751679
return true;
1680+
} else {
1681+
// a built-in function (or some invalid function name)
1682+
auto [normalized, isBuiltIn] =
1683+
Ast::normalizeFunctionName(s.data(), s.size());
1684+
// note: normalizeFunctionName always returns an upper-case name.
1685+
// we must hard-code the function name here, because we can't lookup
1686+
// the definition for the function s without having access to the
1687+
// AqlFunctionsFeature, which is not present here.
1688+
if (normalized == "V8") {
1689+
// the V8() function itself... obviously uses V8
1690+
setFlag(DETERMINED_V8, VALUE_V8);
1691+
return true;
1692+
}
16761693
}
1677-
// fallthrough intentional
1694+
// fallthrough intentional. additionally inspect the function call
1695+
// parameters for CALL/APPLY
16781696
} else {
16791697
// we are unsure about what function will be called by
16801698
// CALL and APPLY. We cannot rule out user-defined functions,
@@ -1686,6 +1704,7 @@ bool AstNode::willUseV8() const {
16861704

16871705
}
16881706

1707+
// inspect all subnodes
16891708
size_t const n = numMembers();
16901709

16911710
for (size_t i = 0; i < n; ++i) {

‎arangod/Aql/ExecutionNode.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1927,7 +1927,7 @@ void CalculationNode::toVelocyPackHelper(VPackBuilder& nodes, unsigned flags,
19271927
nodes.add("canRunOnDBServerOneShard",
19281928
VPackValue(func->hasFlag(Function::Flags::CanRunOnDBServerOneShard)));
19291929
nodes.add("cacheable", VPackValue(func->hasFlag(Function::Flags::Cacheable)));
1930-
nodes.add("usesV8", VPackValue(func->hasV8Implementation()));
1930+
nodes.add("usesV8", VPackValue(node->willUseV8()));
19311931
// deprecated
19321932
nodes.add("canRunOnDBServer",
19331933
VPackValue(func->hasFlag(Function::Flags::CanRunOnDBServerCluster)));

‎arangod/Aql/Expression.cpp

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -949,23 +949,40 @@ AqlValue Expression::executeSimpleExpressionFCallJS(AstNode const* node,
949949
ISOLATE;
950950
TRI_ASSERT(isolate != nullptr);
951951
TRI_V8_CURRENT_GLOBALS_AND_SCOPE;
952-
auto context = TRI_IGETC;
952+
953+
std::string jsName;
954+
if (node->type == NODE_TYPE_FCALL_USER) {
955+
jsName = "FCALL_USER";
956+
} else {
957+
auto func = static_cast<Function*>(node->getData());
958+
TRI_ASSERT(func != nullptr);
959+
TRI_ASSERT(func->hasV8Implementation());
960+
jsName = "AQL_" + func->name;
961+
}
962+
963+
if (v8g == nullptr) {
964+
THROW_ARANGO_EXCEPTION_MESSAGE(
965+
TRI_ERROR_INTERNAL,
966+
std::string(
967+
"no V8 context available when executing call to function ") +
968+
jsName);
969+
}
953970

954971
VPackOptions const& options = _expressionContext->trx().vpackOptions();
955972

956973
auto old = v8g->_expressionContext;
957974
v8g->_expressionContext = _expressionContext;
958975
TRI_DEFER(v8g->_expressionContext = old);
959976

960-
std::string jsName;
961977
size_t const n = member->numMembers();
962978
size_t callArgs = (node->type == NODE_TYPE_FCALL_USER ? 2 : n);
963979
auto args = std::make_unique<v8::Handle<v8::Value>[]>(callArgs);
964980

965981
if (node->type == NODE_TYPE_FCALL_USER) {
966982
// a call to a user-defined function
967-
jsName = "FCALL_USER";
968-
v8::Handle<v8::Array> params = v8::Array::New(isolate, static_cast<int>(n));
983+
auto context = TRI_IGETC;
984+
v8::Handle<v8::Array> params =
985+
v8::Array::New(isolate, static_cast<int>(n));
969986

970987
for (size_t i = 0; i < n; ++i) {
971988
auto arg = member->getMemberUnchecked(i);
@@ -987,7 +1004,6 @@ AqlValue Expression::executeSimpleExpressionFCallJS(AstNode const* node,
9871004
auto func = static_cast<Function*>(node->getData());
9881005
TRI_ASSERT(func != nullptr);
9891006
TRI_ASSERT(func->hasV8Implementation());
990-
jsName = "AQL_" + func->name;
9911007

9921008
for (size_t i = 0; i < n; ++i) {
9931009
auto arg = member->getMemberUnchecked(i);

‎arangod/Aql/Function.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ Function::Function(std::string const& name, char const* arguments,
5858
// currently being able to run on a DB server in cluster always includes being able to run
5959
// on a DB server in OneShard mode. this may change at some point in the future.
6060
TRI_ASSERT(!hasFlag(Flags::CanRunOnDBServerCluster) || hasFlag(Flags::CanRunOnDBServerOneShard));
61+
62+
// only the V8 function does not have a C++ implementation.
63+
// don't ever change this!
64+
// note: CUSTOMSCORER and INVALID are only used by unit tests
65+
TRI_ASSERT(hasCxxImplementation() || name == "V8" || name == "CUSTOMSCORER" ||
66+
name == "INVALID");
6167
}
6268

6369
#ifdef ARANGODB_USE_GOOGLE_TESTS

‎arangod/Aql/Functions.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@
8686
#include "utils/math_utils.hpp"
8787
#include "utils/ngram_match_utils.hpp"
8888

89+
#include <absl/strings/str_cat.h>
90+
8991
#include <boost/uuid/uuid.hpp>
9092
#include <boost/uuid/uuid_generators.hpp>
9193
#include <boost/uuid/uuid_io.hpp>
@@ -1109,6 +1111,12 @@ AqlValue callApplyBackend(ExpressionContext* expressionContext, AstNode const& n
11091111
// JavaScript function (this includes user-defined functions)
11101112
{
11111113
ISOLATE;
1114+
if (isolate == nullptr) {
1115+
THROW_ARANGO_EXCEPTION_MESSAGE(
1116+
TRI_ERROR_INTERNAL,
1117+
absl::StrCat(
1118+
"no V8 context available when executing call to function ", AFN));
1119+
}
11121120
TRI_V8_CURRENT_GLOBALS_AND_SCOPE;
11131121
auto context = TRI_IGETC;
11141122

‎arangod/Transaction/V8Context.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@
2727
#include "Transaction/StandaloneContext.h"
2828
#include "Utils/CollectionNameResolver.h"
2929
#include "V8Server/V8DealerFeature.h"
30+
#include "V8/v8-globals.h"
3031

3132
#include <v8.h>
32-
#include "V8/v8-globals.h"
3333

3434
using namespace arangodb;
3535

@@ -103,6 +103,11 @@ CollectionNameResolver const& transaction::V8Context::resolver() {
103103
void transaction::V8Context::enterV8Context() {
104104
// registerTransaction
105105
auto v8g = getV8State();
106+
if (v8g == nullptr) {
107+
THROW_ARANGO_EXCEPTION_MESSAGE(
108+
TRI_ERROR_INTERNAL,
109+
"no v8 context available to enter for current transaction context");
110+
}
106111
TRI_ASSERT(v8g != nullptr);
107112

108113
TRI_ASSERT(_currentTransaction != nullptr);

‎tests/js/common/aql/aql-view-arangosearch-cluster.inc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@
240240

241241
testV8FunctionInSearch: function () {
242242
try {
243-
db._query("FOR doc IN CompoundView SEARCH doc.a == 'foo' && APPLY('LOWER', 'abc') == 'ABC' OPTIONS { waitForSync: true } RETURN doc");
243+
db._query("FOR doc IN CompoundView SEARCH doc.a == 'foo' && V8('abc') == 'ABC' OPTIONS { waitForSync: true } RETURN doc");
244244
fail();
245245
} catch (e) {
246246
assertEqual(ERRORS.ERROR_NOT_IMPLEMENTED.code, e.errorNum);

‎tests/js/common/aql/aql-view-arangosearch-noncluster.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ function iResearchAqlTestSuite () {
203203

204204
testV8FunctionInSearch: function () {
205205
try {
206-
db._query("FOR doc IN CompoundView SEARCH doc.a == 'foo' && APPLY('LOWER', 'abc') == 'ABC' OPTIONS { waitForSync: true } RETURN doc");
206+
db._query("FOR doc IN CompoundView SEARCH doc.a == 'foo' && V8('abc') == 'ABC' OPTIONS { waitForSync: true } RETURN doc");
207207
fail();
208208
} catch (e) {
209209
assertEqual(ERRORS.ERROR_NOT_IMPLEMENTED.code, e.errorNum);

0 commit comments

Comments
 (0)
0