From d214c7583e4b1eb546a13410be3901e110e5fd6e Mon Sep 17 00:00:00 2001 From: ArthurPV Date: Sun, 14 Apr 2024 01:31:24 -0400 Subject: [PATCH 1/6] Fix issue by using `consume` with an invalid expression --- src/libponyc/pass/refer.c | 57 ++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/src/libponyc/pass/refer.c b/src/libponyc/pass/refer.c index 05fdc6b2ba..be71967213 100644 --- a/src/libponyc/pass/refer.c +++ b/src/libponyc/pass/refer.c @@ -83,7 +83,7 @@ static bool is_this_incomplete(pass_opt_t* opt, ast_t* ast) // is used to get the definition for the type based on the `ast_data` to ensure // at some point the field is tied to a real type even if we haven't quite fully // determined the type of each field/subfield reference yet. -static const char* generate_multi_dot_name(ast_t* ast, ast_t** def_found) { +static const char* generate_multi_dot_name(pass_opt_t *opt, ast_t* ast, ast_t** def_found, bool in_consume_ctx, bool *has_consume_error) { pony_assert(ast_id(ast) == TK_DOT); ast_t* def = NULL; @@ -141,6 +141,20 @@ static const char* generate_multi_dot_name(ast_t* ast, ast_t** def_found) { { if (def == NULL) return stringtab(""); + else if (in_consume_ctx) { + pony_assert(has_consume_error); + + *has_consume_error = true; + + ast_error(opt->check.errors, temp_ast, + "You can't consume an expression that isn't local. More" + " specifically, you can only consume a local variable (a" + " single lowercase identifier, with no dots) or a field" + " of this (this followed by a dot and a single lowercase" + " identifier)."); + return stringtab(""); + } + pony_assert(0); } } @@ -181,7 +195,7 @@ static const char* generate_multi_dot_name(ast_t* ast, ast_t** def_found) { return stringtab_consume(buf, len); } -static bool is_matching_assign_lhs(ast_t* a, ast_t* b) +static bool is_matching_assign_lhs(pass_opt_t *opt, ast_t* a, ast_t* b) { // Has to be the left hand side of an assignment (the first child). if(a == b) @@ -191,8 +205,8 @@ static bool is_matching_assign_lhs(ast_t* a, ast_t* b) if((ast_id(a) == TK_DOT) && (ast_id(b) == TK_DOT)) { // get fully qualified string identifier without `this` - const char* a_name = generate_multi_dot_name(a, NULL); - const char* b_name = generate_multi_dot_name(b, NULL); + const char* a_name = generate_multi_dot_name(opt, a, NULL, false, NULL); + const char* b_name = generate_multi_dot_name(opt, b, NULL, false, NULL); if(a_name == b_name) return true; @@ -201,7 +215,7 @@ static bool is_matching_assign_lhs(ast_t* a, ast_t* b) return false; } -static bool is_assigned_to(ast_t* ast, bool check_result_needed) +static bool is_assigned_to(pass_opt_t *opt, ast_t* ast, bool check_result_needed) { while(true) { @@ -211,7 +225,7 @@ static bool is_assigned_to(ast_t* ast, bool check_result_needed) { case TK_ASSIGN: { - if(!is_matching_assign_lhs(ast_child(parent), ast)) + if(!is_matching_assign_lhs(opt, ast_child(parent), ast)) return false; if(!check_result_needed) @@ -309,7 +323,7 @@ static bool valid_reference(pass_opt_t* opt, ast_t* ast, sym_status_t status) case SYM_CONSUMED: case SYM_CONSUMED_SAME_EXPR: - if(is_assigned_to(ast, true)) + if(is_assigned_to(opt, ast, true)) return true; ast_error(opt->check.errors, ast, @@ -317,7 +331,7 @@ static bool valid_reference(pass_opt_t* opt, ast_t* ast, sym_status_t status) return false; case SYM_UNDEFINED: - if(is_assigned_to(ast, true)) + if(is_assigned_to(opt, ast, true)) return true; ast_error(opt->check.errors, ast, @@ -638,7 +652,7 @@ static bool refer_multi_dot(pass_opt_t* opt, ast_t* ast) AST_GET_CHILDREN(ast, left, right); // get fully qualified string identifier without `this` - const char* name = generate_multi_dot_name(ast, NULL); + const char* name = generate_multi_dot_name(opt, ast, NULL, false, NULL); // use this string to check status using `valid_reference` function. sym_status_t status; @@ -766,7 +780,7 @@ static lvalue_t assign_multi_dot(pass_opt_t* opt, ast_t* ast, bool need_value) pony_assert(ast_id(ast) == TK_DOT); // get fully qualified string identifier without `this` - const char* name = generate_multi_dot_name(ast, NULL); + const char* name = generate_multi_dot_name(opt, ast, NULL, false, NULL); sym_status_t status; ast_get(ast, name, &status); @@ -1049,7 +1063,7 @@ static bool refer_assign(pass_opt_t* opt, ast_t* ast) return true; } -static bool ast_get_child(ast_t* ast, const char* name) +static bool ast_get_child(pass_opt_t *opt, ast_t* ast, const char* name) { const char* assign_name = NULL; @@ -1064,7 +1078,7 @@ static bool ast_get_child(ast_t* ast, const char* name) case TK_DOT: { // get fully qualified string identifier without `this` - assign_name = generate_multi_dot_name(ast, NULL); + assign_name = generate_multi_dot_name(opt, ast, NULL, false, NULL); break; } @@ -1079,7 +1093,7 @@ static bool ast_get_child(ast_t* ast, const char* name) while(child != NULL) { - if(ast_get_child(child, name)) + if(ast_get_child(opt, child, name)) return true; child = ast_sibling(child); @@ -1088,7 +1102,7 @@ static bool ast_get_child(ast_t* ast, const char* name) return false; } -static bool check_assigned_same_expression(ast_t* ast, const char* name, +static bool check_assigned_same_expression(pass_opt_t *opt, ast_t* ast, const char* name, ast_t** ret_assign_ast) { ast_t* assign_ast = ast; @@ -1101,7 +1115,7 @@ static bool check_assigned_same_expression(ast_t* ast, const char* name, return false; ast_t* assign_left = ast_child(assign_ast); - return ast_get_child(assign_left, name); + return ast_get_child(opt, assign_left, name); } static void set_flag_recursive(ast_t* outer, uint32_t flag) @@ -1137,7 +1151,7 @@ static bool refer_consume(pass_opt_t* opt, ast_t* ast) ast_t* id = ast_child(term); name = ast_name(id); ast_t* assign_ast = NULL; - if(check_assigned_same_expression(id, name, &assign_ast)) + if(check_assigned_same_expression(opt, id, name, &assign_ast)) { consumed_same_expr = true; ast_setflag(assign_ast, AST_FLAG_CNSM_REASGN); @@ -1157,6 +1171,7 @@ static bool refer_consume(pass_opt_t* opt, ast_t* ast) AST_GET_CHILDREN(term, left, right); ast_t* def = NULL; + bool has_consume_error = false; if(ast_id(left) == TK_THIS) { @@ -1175,14 +1190,14 @@ static bool refer_consume(pass_opt_t* opt, ast_t* ast) { // get fully qualified string identifier without `this` // and def of the root object - name = generate_multi_dot_name(term, &def); + name = generate_multi_dot_name(opt, term, &def, true, &has_consume_error); // defer checking it's not a let or embed if it's not a `this` variable // because we don't have the type info available. The expr pass will // catch it in the `expr_consume` function. } - if(def == NULL) + if(def == NULL && !has_consume_error) { ast_error(opt->check.errors, ast, "cannot consume an unknown field type"); @@ -1191,7 +1206,7 @@ static bool refer_consume(pass_opt_t* opt, ast_t* ast) ast_t* assign_ast = NULL; - if(!check_assigned_same_expression(ast, name, &assign_ast)) + if(!check_assigned_same_expression(opt, ast, name, &assign_ast)) { ast_error(opt->check.errors, ast, "consuming a field is only allowed if it is reassigned in the same" @@ -1274,7 +1289,7 @@ static bool refer_local(pass_opt_t* opt, ast_t* ast) if(ast_id(type) == TK_NONE) { // No type specified, infer it later - if(!is_dontcare && !is_assigned_to(ast, false)) + if(!is_dontcare && !is_assigned_to(opt, ast, false)) { ast_error(opt->check.errors, ast, "locals must specify a type or be assigned a value"); @@ -1284,7 +1299,7 @@ static bool refer_local(pass_opt_t* opt, ast_t* ast) else if(ast_id(ast) == TK_LET) { // Let, check we have a value assigned - if(!is_assigned_to(ast, false)) + if(!is_assigned_to(opt, ast, false)) { ast_error(opt->check.errors, ast, "can't declare a let local without assigning to it"); From 1eff3bb7814e9659e520fa87970929e49444213f Mon Sep 17 00:00:00 2001 From: ArthurPV Date: Sun, 14 Apr 2024 01:41:00 -0400 Subject: [PATCH 2/6] Test the changes that resolve the issue #4477 --- test/libponyc/refer.cc | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/libponyc/refer.cc b/test/libponyc/refer.cc index 89581d403e..e0ee127588 100644 --- a/test/libponyc/refer.cc +++ b/test/libponyc/refer.cc @@ -18,6 +18,10 @@ { const char* errs[] = {err1, err2, err3, NULL}; \ DO(test_expected_errors(src, "refer", errs)); } +#define TEST_ERRORS_4(src, err1, err2, err3, err4) \ + { const char* errs[] = {err1, err2, err3, err4, NULL}; \ + DO(test_expected_errors(src, "refer", errs)); } + class ReferTest : public PassTest {}; @@ -884,6 +888,35 @@ TEST_F(ReferTest, ConsumeAfterMemberAccessWithConsumeLhs) "consume must take 'this', a local, or a parameter"); } +TEST_F(ReferTest, ConsumeInvalidExpression) +{ + // From issue #4477 + const char *src = + "struct FFIBytes\n" + "var ptr: Pointer[U8 val] = Pointer[U8].create()\n" + "var length: USize = 0\n" + "fun iso string(): String val =>\n" + "recover String.from_cpointer(consume FFIBytes.ptr," + "consume FFIBytes.length) end\n" + "actor Main\n" + "new create(env: Env) =>\n" + "env.out.print(\"nothing to see here\")\n"; + + TEST_ERRORS_4(src, + "You can't consume an expression that isn't local. More specifically," + " you can only consume a local variable (a single lowercase" + " identifier, with no dots) or a field of this (this followed by a dot" + " and a single lowercase identifier).", + "consuming a field is only allowed if it is reassigned in the same" + " expression", + "You can't consume an expression that isn't local. More specifically," + " you can only consume a local variable (a single lowercase" + " identifier, with no dots) or a field of this (this followed by a dot" + " and a single lowercase identifier).", + "consuming a field is only allowed if it is reassigned in the same" + " expression"); +} + TEST_F(ReferTest, MemberAccessWithConsumeLhs) { const char* src = From ee0e5f59ffe9655bac79a3d5b345456e95e211ac Mon Sep 17 00:00:00 2001 From: ArthurPV Date: Sun, 14 Apr 2024 11:00:40 -0400 Subject: [PATCH 3/6] Add a release notes. --- .release-notes/4505.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .release-notes/4505.md diff --git a/.release-notes/4505.md b/.release-notes/4505.md new file mode 100644 index 0000000000..81fdbd6a2c --- /dev/null +++ b/.release-notes/4505.md @@ -0,0 +1,10 @@ +## Fix: Segmentation fault when ponyc compiles this code + +Fix an issue caused by passing a wrong expression to `consume`. + +With this change, instead of the compiler generating a +segmentation fault, it generates the error message: "You cannot +consume an expression that is not local. Specifically, you can +only consume a local variable (a single lowercase identifier, without +a dot) or a this field (this followed by a dot and a single lowercase +identifier)". From 0aab38d4513bf1de4b1b47f839e9c97ccc95aae2 Mon Sep 17 00:00:00 2001 From: Sean T Allen Date: Tue, 16 Apr 2024 14:09:50 -0400 Subject: [PATCH 4/6] Update .release-notes/4505.md --- .release-notes/4505.md | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/.release-notes/4505.md b/.release-notes/4505.md index 81fdbd6a2c..b94b03b23e 100644 --- a/.release-notes/4505.md +++ b/.release-notes/4505.md @@ -1,10 +1,15 @@ -## Fix: Segmentation fault when ponyc compiles this code +## Fix compiler segmentation fault -Fix an issue caused by passing a wrong expression to `consume`. +Previously the following code would cause the compiler to crash. Now it will display a helpful error message: -With this change, instead of the compiler generating a -segmentation fault, it generates the error message: "You cannot -consume an expression that is not local. Specifically, you can -only consume a local variable (a single lowercase identifier, without -a dot) or a this field (this followed by a dot and a single lowercase -identifier)". +```pony +struct FFIBytes + var ptr: Pointer[U8 val] = Pointer[U8].create() + var length: USize = 0 + + fun iso string(): String val => + recover String.from_cpointer(consume FFIBytes.ptr, consume FFIBytes.length) end + +actor Main + new create(env: Env) => + env.out.print("nothing to see here") From 1b8735457a7955337d25370806a322426ef10d25 Mon Sep 17 00:00:00 2001 From: Sean T Allen Date: Tue, 16 Apr 2024 14:10:23 -0400 Subject: [PATCH 5/6] Update .release-notes/4505.md --- .release-notes/4505.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-notes/4505.md b/.release-notes/4505.md index b94b03b23e..4f6727bc48 100644 --- a/.release-notes/4505.md +++ b/.release-notes/4505.md @@ -1,4 +1,4 @@ -## Fix compiler segmentation fault +## Fix compiler crash Previously the following code would cause the compiler to crash. Now it will display a helpful error message: From 7a5df361be143fe96190883394edda2c03ecf980 Mon Sep 17 00:00:00 2001 From: Sean T Allen Date: Tue, 16 Apr 2024 14:13:46 -0400 Subject: [PATCH 6/6] Update 4505.md --- .release-notes/4505.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.release-notes/4505.md b/.release-notes/4505.md index 4f6727bc48..f894d2c700 100644 --- a/.release-notes/4505.md +++ b/.release-notes/4505.md @@ -13,3 +13,4 @@ struct FFIBytes actor Main new create(env: Env) => env.out.print("nothing to see here") +```