Skip to content
Permalink
Browse files Browse the repository at this point in the history
Remove patch and merge in_place optimizations.
They are too dangerous and can lead to memory unsafety.

Also added proper state handling to script based tests.

Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
  • Loading branch information
mfelsche committed Sep 15, 2021
1 parent bffc2a5 commit 1a2efcd
Show file tree
Hide file tree
Showing 19 changed files with 40 additions and 152 deletions.
5 changes: 3 additions & 2 deletions tests/script.rs
Expand Up @@ -51,6 +51,7 @@ macro_rules! test_cases {
out_json.reverse();

let mut results = Vec::new();
let mut state = Value::null();
for (id, mut json) in in_json.into_iter().enumerate() {
let uri = EventOriginUri{
host: "test".into(),
Expand All @@ -62,7 +63,6 @@ macro_rules! test_cases {
};
let context = EventContext::new(id as u64, Some(&uri));
let mut meta = Value::from(Object::default());
let mut state = Value::null();
match script.run(&context, AggrType::Tick, &mut json, &mut state, &mut meta)? {
Return::Drop => (),
Return::EmitEvent{..} => results.push(json),
Expand Down Expand Up @@ -154,6 +154,7 @@ test_cases!(
// TODO
// const_in_const_lookup,
// INSERT
merge_assign_target_state,
expr_path,
patch_default,
patch_default_key,
Expand Down Expand Up @@ -186,7 +187,7 @@ test_cases!(
heredoc_quoted_curly,
string_interpolation_import,
string_interpolation_prefix,
patch_in_place,
patch_assign_target,
tuple_pattern,
pattern_cmp,
pass_args,
Expand Down
6 changes: 3 additions & 3 deletions tests/script_runtime_error.rs
Expand Up @@ -105,10 +105,10 @@ macro_rules! ignore_cases {
file.read_to_string(&mut err)?;
let _err = err.trim();

let mut state = Value::null();
if let Some(mut json) = in_json.pop() {
let context = EventContext::new(0, None);
let mut meta = Value::object();
let mut state = Value::null();
let s = script.run(&context, AggrType::Tick, &mut json, &mut state, &mut meta);
if let Err(e) = s {
let mut h = Dumb::new();
Expand Down Expand Up @@ -144,8 +144,8 @@ test_cases!(
function_error_n,
match_bad_guard_type,
match_no_clause_hit,
merge_in_place_new_no_object,
merge_in_place_target_no_object,
merge_assign_target_new_no_object,
merge_assign_target_target_no_object,
merge_new_no_object,
merge_target_no_object,
missing_local,
Expand Down
2 changes: 2 additions & 0 deletions tests/scripts/merge_assign_target_state/in
@@ -0,0 +1,2 @@
{"foo": "bar"}
{"snot": "badger", "foo": "grmpf"}
2 changes: 2 additions & 0 deletions tests/scripts/merge_assign_target_state/out
@@ -0,0 +1,2 @@
{"foo":"bar"}
{"foo":"grmpf","snot":"badger"}
8 changes: 8 additions & 0 deletions tests/scripts/merge_assign_target_state/script.tremor
@@ -0,0 +1,8 @@
# initialize state to record, so we can do the merge
let state = match state of
case null => {}
default => state
end;

let state = merge state of event end;
emit state
File renamed without changes.
File renamed without changes.
File renamed without changes.
4 changes: 0 additions & 4 deletions tremor-script/src/ast.rs
Expand Up @@ -961,10 +961,6 @@ pub enum Expr<'script> {
Match(Box<Match<'script, Self>>),
/// IfElse style match expression
IfElse(Box<IfElse<'script, Self>>),
/// In place patch expression
PatchInPlace(Box<Patch<'script>>),
/// In place merge expression
MergeInPlace(Box<Merge<'script>>),
/// Assignment expression
Assign {
/// Id
Expand Down
2 changes: 0 additions & 2 deletions tremor-script/src/ast/base_expr.rs
Expand Up @@ -248,8 +248,6 @@ impl<'script> BaseExpr for Expr<'script> {
Expr::Emit(e) => e.mid(),
Expr::Imut(e) => e.mid(),
Expr::Match(e) => e.mid(),
Expr::MergeInPlace(e) => e.mid(),
Expr::PatchInPlace(e) => e.mid(),
Expr::IfElse(e) => e.mid(),
}
}
Expand Down
48 changes: 18 additions & 30 deletions tremor-script/src/ast/raw.rs
Expand Up @@ -18,14 +18,14 @@

use crate::{
ast::{
base_expr, eq::AstEq, query, upable::Upable, ArrayPattern, ArrayPredicatePattern,
AssignPattern, BinExpr, BinOpKind, Bytes, BytesPart, ClauseGroup, Comprehension,
ComprehensionCase, Costly, DefaultCase, EmitExpr, EventPath, Expr, ExprPath, Expression,
Field, FnDecl, FnDoc, Helper, Ident, IfElse, ImutExpr, ImutExprInt, Invocable, Invoke,
InvokeAggr, InvokeAggrFn, List, Literal, LocalPath, Match, Merge, MetadataPath, ModDoc,
NodeMetas, Patch, PatchOperation, Path, Pattern, PredicateClause, PredicatePattern, Record,
RecordPattern, Recur, ReservedPath, Script, Segment, StatePath, StrLitElement, StringLit,
TestExpr, TuplePattern, UnaryExpr, UnaryOpKind,
base_expr, query, upable::Upable, ArrayPattern, ArrayPredicatePattern, AssignPattern,
BinExpr, BinOpKind, Bytes, BytesPart, ClauseGroup, Comprehension, ComprehensionCase,
Costly, DefaultCase, EmitExpr, EventPath, Expr, ExprPath, Expression, Field, FnDecl, FnDoc,
Helper, Ident, IfElse, ImutExpr, ImutExprInt, Invocable, Invoke, InvokeAggr, InvokeAggrFn,
List, Literal, LocalPath, Match, Merge, MetadataPath, ModDoc, NodeMetas, Patch,
PatchOperation, Path, Pattern, PredicateClause, PredicatePattern, Record, RecordPattern,
Recur, ReservedPath, Script, Segment, StatePath, StrLitElement, StringLit, TestExpr,
TuplePattern, UnaryExpr, UnaryOpKind,
},
errors::{
err_generic, error_generic, error_missing_effector, error_oops, Error, ErrorKind, Result,
Expand Down Expand Up @@ -646,28 +646,16 @@ impl<'script> Upable<'script> for ExprRaw<'script> {
let path = a.path.up(helper)?;
let mid = helper.add_meta(a.start, a.end);
match a.expr.up(helper)? {
Expr::Imut(ImutExprInt::Merge(m)) => {
if path.ast_eq(&m.target) {
Expr::MergeInPlace(Box::new(*m))
} else {
Expr::Assign {
mid,
path,
expr: Box::new(ImutExprInt::Merge(m).into()),
}
}
}
Expr::Imut(ImutExprInt::Patch(m)) => {
if path.ast_eq(&m.target) {
Expr::PatchInPlace(Box::new(*m))
} else {
Expr::Assign {
mid,
path,
expr: Box::new(ImutExprInt::Patch(m).into()),
}
}
}
Expr::Imut(ImutExprInt::Merge(m)) => Expr::Assign {
mid,
path,
expr: Box::new(ImutExprInt::Merge(m).into()),
},
Expr::Imut(ImutExprInt::Patch(m)) => Expr::Assign {
mid,
path,
expr: Box::new(ImutExprInt::Patch(m).into()),
},
expr => Expr::Assign {
mid,
path,
Expand Down
2 changes: 0 additions & 2 deletions tremor-script/src/ast/to_static.rs
Expand Up @@ -159,8 +159,6 @@ impl<'script> Expr<'script> {
match self {
Expr::Match(e) => Expr::Match(Box::new(e.into_static())),
Expr::IfElse(e) => Expr::IfElse(Box::new(e.into_static())),
Expr::PatchInPlace(e) => Expr::PatchInPlace(Box::new(e.into_static())),
Expr::MergeInPlace(e) => Expr::MergeInPlace(Box::new(e.into_static())),
Expr::Assign { mid, path, expr } => Expr::Assign {
mid,
path: path.into_static(),
Expand Down
113 changes: 4 additions & 109 deletions tremor-script/src/interpreter/expr.rs
Expand Up @@ -13,24 +13,23 @@
// limitations under the License.

use super::{
merge_values, patch_value, resolve, resolve_value, set_local_shadow, test_guard,
test_predicate_expr, Env, ExecOpts, LocalStack, NULL,
resolve, resolve_value, set_local_shadow, test_guard, test_predicate_expr, Env, ExecOpts,
LocalStack, NULL,
};
use crate::errors::{
error_assign_array, error_assign_to_const, error_bad_key_err, error_invalid_assign_target,
error_need_obj, error_need_obj_err, error_no_clause_hit, Result,
error_need_obj_err, error_no_clause_hit, Result,
};
use crate::prelude::*;
use crate::registry::RECUR_PTR;
use crate::{
ast::{
BaseExpr, ClauseGroup, ClausePreCondition, Comprehension, DefaultCase, EmitExpr, EventPath,
Expr, IfElse, ImutExprInt, Match, Merge, Patch, Path, Segment,
Expr, IfElse, ImutExprInt, Match, Path, Segment,
},
errors::error_oops_err,
};
use crate::{stry, Value};
use matches::matches;
use std::mem;
use std::{
borrow::{Borrow, Cow},
Expand Down Expand Up @@ -219,104 +218,6 @@ impl<'script> Expr<'script> {
}
}

fn patch_in_place<'run, 'event>(
opts: ExecOpts,
env: &'run Env<'run, 'event>,
event: &'run Value<'event>,
state: &'run Value<'static>,
meta: &'run Value<'event>,
local: &'run LocalStack<'event>,
expr: &'run Patch<'event>,
) -> Result<Cow<'run, Value<'event>>> {
// This function is called when we encounter code that consumes a value
// to patch it. So the following code:
// ```tremor
// let event = patch event of insert "key" => "value" end
// ```
// When executed on it's own would clone the event, add a key and
// overwrite original event.
//
// We optimise this as:
// ```
// patch_in_place event of insert "key" => "value" end
// ```
//
// This code is generated in impl Upable for ExprRaw where the following
// checks are performed:
//
// 1) the patch is on the RHS of an assignment
// 2) the path of the assigned value and the path of the patched
// expression are identical.
//
// In turn this guarantees (at compile time):
//
// 1) The target (`expr`) is a path lookup
// 2) The target is not a known constant as otherwise the assignment
// will complan
// 3) this leave the `expr` to be either a local, the event, the state,
// metadata or a subkey thereof.
//
// And the following guarantees at run time:
//
// 1) the `expr` is an existing key of the mentioned categories,
// otherwise `expr.target.run` will error.
// 2) `value` will never be owned (however the resolve function is
// generic so it needs to return a Cow)

let value: Cow<'run, Value<'event>> =
stry!(expr.target.run(opts, env, event, state, meta, local));
debug_assert!(
!matches!(value, Cow::Owned(_)),
"We should never see a owned value here as patch_in_place is only ever called on existing data in event, state, meta or local"
);
let v: &Value<'event> = value.borrow();
// ALLOW: https://github.com/tremor-rs/tremor-runtime/issues/1032
#[allow(mutable_transmutes, clippy::transmute_ptr_to_ptr)]
// ALLOW: https://github.com/tremor-rs/tremor-runtime/issues/1032
let v: &mut Value<'event> = unsafe { mem::transmute(v) };
stry!(patch_value(opts, env, event, state, meta, local, v, expr));
Ok(value)
}

fn merge_in_place<'run, 'event>(
&'run self,
opts: ExecOpts,
env: &'run Env<'run, 'event>,
event: &'run mut Value<'event>,
state: &'run mut Value<'static>,
meta: &'run mut Value<'event>,
local: &'run mut LocalStack<'event>,
expr: &'run Merge<'event>,
) -> Result<Cow<'run, Value<'event>>> {
// Please see the soundness reasoning in `patch_in_place` for details
// those functions perform the same function just with slighty different
// operations.
let value_cow: Cow<'run, Value<'event>> =
stry!(expr.target.run(opts, env, event, state, meta, local));
debug_assert!(
!matches!(value_cow, Cow::Owned(_)),
"We should never see a owned value here as merge_in_place is only ever called on existing data in event, state, meta or local"
);

if value_cow.is_object() {
let value: &Value<'event> = value_cow.borrow();
// ALLOW: https://github.com/tremor-rs/tremor-runtime/issues/1032
#[allow(mutable_transmutes, clippy::transmute_ptr_to_ptr)]
// ALLOW: https://github.com/tremor-rs/tremor-runtime/issues/1032
let value: &mut Value<'event> = unsafe { mem::transmute(value) };
let replacement = stry!(expr.expr.run(opts, env, event, state, meta, local,));

if replacement.is_object() {
stry!(merge_values(self, &expr.expr, value, &replacement));
Ok(value_cow)
} else {
error_need_obj(self, &expr.expr, replacement.value_type(), env.meta)
}
} else {
error_need_obj(self, &expr.target, value_cow.value_type(), env.meta)
}
}

// TODO: Quite some overlap with `ImutExprInt::comprehension`
fn comprehension<'run, 'event>(
&'run self,
Expand Down Expand Up @@ -641,12 +542,6 @@ impl<'script> Expr<'script> {
}
Expr::Match(ref expr) => self.match_expr(opts, env, event, state, meta, local, expr),
Expr::IfElse(ref expr) => self.if_expr(opts, env, event, state, meta, local, expr),
Expr::MergeInPlace(ref expr) => self
.merge_in_place(opts, env, event, state, meta, local, expr)
.map(Cont::Cont),
Expr::PatchInPlace(ref expr) => {
Self::patch_in_place(opts, env, event, state, meta, local, expr).map(Cont::Cont)
}
Expr::Comprehension(ref expr) => {
self.comprehension(opts, env, event, state, meta, local, expr)
}
Expand Down

0 comments on commit 1a2efcd

Please sign in to comment.