From 0c1227d53bdc0f304dc2cd00d75be6a001d8b0f7 Mon Sep 17 00:00:00 2001 From: Michael Haselberger Date: Wed, 23 Oct 2024 14:07:02 +0200 Subject: [PATCH] validate dangling pointers --- .../complex_initializers.rs | 40 +++----- src/lowering.rs | 92 ++++++++++--------- .../tests/variable_validation_tests.rs | 63 +++++++++++++ src/validation/variable.rs | 66 +++++++++++-- tests/lit/single/init/function_locals.st | 27 ++++++ 5 files changed, 210 insertions(+), 78 deletions(-) diff --git a/src/codegen/tests/initialization_test/complex_initializers.rs b/src/codegen/tests/initialization_test/complex_initializers.rs index cf4f090ea9..ccb21b866b 100644 --- a/src/codegen/tests/initialization_test/complex_initializers.rs +++ b/src/codegen/tests/initialization_test/complex_initializers.rs @@ -1590,16 +1590,16 @@ fn temporary_variable_ref_to_local_member() { fn temporary_variable_ref_to_temporary_variable() { let res = codegen( r" - FUNCTION_BLOCK foo + FUNCTION foo VAR + ptr : REF_TO STRING := REF(s); + alias AT s : STRING; END_VAR VAR_TEMP s : STRING; - ptr : REF_TO STRING := REF(s); - alias AT s : STRING; - reference_to : REFERENCE TO STRING REF= s; + reference_to : REFERENCE TO STRING REF= alias; END_VAR - END_FUNCTION_BLOCK + FUNCTION ", ); @@ -1607,24 +1607,21 @@ fn temporary_variable_ref_to_temporary_variable() { ; ModuleID = '' source_filename = "" - %foo = type {} - - @__foo__init = unnamed_addr constant %foo zeroinitializer - - define void @foo(%foo* %0) { + define void @foo() { entry: - %s = alloca [81 x i8], align 1 %ptr = alloca [81 x i8]*, align 8 %alias = alloca [81 x i8]*, align 8 + %s = alloca [81 x i8], align 1 %reference_to = alloca [81 x i8]*, align 8 - %1 = bitcast [81 x i8]* %s to i8* - call void @llvm.memset.p0i8.i64(i8* align 1 %1, i8 0, i64 ptrtoint ([81 x i8]* getelementptr ([81 x i8], [81 x i8]* null, i32 1) to i64), i1 false) store [81 x i8]* %s, [81 x i8]** %ptr, align 8 store [81 x i8]* null, [81 x i8]** %alias, align 8 + %0 = bitcast [81 x i8]* %s to i8* + call void @llvm.memset.p0i8.i64(i8* align 1 %0, i8 0, i64 ptrtoint ([81 x i8]* getelementptr ([81 x i8], [81 x i8]* null, i32 1) to i64), i1 false) store [81 x i8]* null, [81 x i8]** %reference_to, align 8 store [81 x i8]* %s, [81 x i8]** %ptr, align 8 store [81 x i8]* %s, [81 x i8]** %alias, align 8 - store [81 x i8]* %s, [81 x i8]** %reference_to, align 8 + %deref = load [81 x i8]*, [81 x i8]** %alias, align 8 + store [81 x i8]* %deref, [81 x i8]** %reference_to, align 8 ret void } @@ -1632,21 +1629,6 @@ fn temporary_variable_ref_to_temporary_variable() { declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg) #0 attributes #0 = { argmemonly nofree nounwind willreturn writeonly } - ; ModuleID = '__initializers' - source_filename = "__initializers" - - %foo = type {} - - @__foo__init = external global %foo - - define void @__init_foo(%foo* %0) { - entry: - %self = alloca %foo*, align 8 - store %foo* %0, %foo** %self, align 8 - ret void - } - - declare void @foo(%foo*) ; ModuleID = '__init___testproject' source_filename = "__init___testproject" diff --git a/src/lowering.rs b/src/lowering.rs index 20251e867f..48b58d301b 100644 --- a/src/lowering.rs +++ b/src/lowering.rs @@ -85,37 +85,41 @@ impl AstLowerer { // flat references to stateful pou-local variables need to have a qualifier added, so they can be resolved in the init functions let scope = self.ctxt.get_scope().as_ref().map(|it| it.as_str()).unwrap_or(GLOBAL_SCOPE); let needs_qualifier = |flat_ref| { - let rhs = self.index.find_local_member(scope, flat_ref); - let lhs = self.index.find_local_member(scope, variable.get_name()); - if lhs.is_some_and(|it| !it.is_temp()) && rhs.is_some_and(|it| it.is_temp()) { - // TODO: diagnostic - eprintln!("Assigning an address of a temporary variable to a stateful member variable can result in use-after-free"); - return false; + let rhs = self.index.find_member(scope, flat_ref); + let lhs = self.index.find_member(scope, variable.get_name()); + let Some(pou) = self.index.find_pou(scope) else { return Ok(false) }; + if !pou.is_function() && lhs.is_some_and(|it| !it.is_temp()) && rhs.is_some_and(|it| it.is_temp()) + { + // Unable to initialize a stateful member variable with an address of a temporary value since it doesn't exist at the time of initialization + // On top of that, even if we were to initialize it, it would lead to a dangling pointer/potential use-after-free + return Err(AstFactory::create_empty_statement( + SourceLocation::internal(), + self.ctxt.get_id_provider().next_id(), + )); } - self.index - .find_pou(scope) - .map(|it| match it { - PouIndexEntry::Program { .. } - | PouIndexEntry::FunctionBlock { .. } - | PouIndexEntry::Class { .. } - // we only want to add qualifiers to local, non-temporary variables - if rhs.is_some_and(|it| !it.is_temp()) && lhs.is_some_and(|it| !it.is_temp())=> - { - true - } - PouIndexEntry::Method { .. } => { - unimplemented!("We'll worry about this once we get around to OOP") - } - _ => false, - }) - .unwrap_or_default() + Ok(match pou { + PouIndexEntry::Program { .. } + | PouIndexEntry::FunctionBlock { .. } + | PouIndexEntry::Class { .. } + // we only want to add qualifiers to local, non-temporary variables + if rhs.is_some_and(|it| !it.is_temp()) && lhs.is_some_and(|it| !it.is_temp())=> + { + true + } + PouIndexEntry::Method { .. } => { + unimplemented!("We'll worry about this once we get around to OOP") + } + _ => false, + }) }; if let Some(initializer) = variable.initializer.as_ref() { - let hint = self.annotations.get_type_hint(initializer, &self.index); - if hint + if self + .annotations + .get_type_hint(initializer, &self.index) .map(|it| it.get_type_information()) - .is_some_and(|it| !(it.is_pointer() || it.is_reference_to() || it.is_alias())) + .filter(|dti| dti.is_pointer() || dti.is_reference_to() || dti.is_alias()) + .is_none() { return; }; @@ -123,29 +127,33 @@ impl AstLowerer { // no call-statement in the initializer, so something like `a AT b` or `a : REFERENCE TO ... REF= b` AstStatement::ReferenceExpr(_) => { initializer.get_flat_reference_name().and_then(|flat_ref| { - needs_qualifier(flat_ref) - .then_some("self") - .map(|it| create_member_reference(it, self.ctxt.get_id_provider(), None)) - .and_then(|base| { - initializer.get_flat_reference_name().map(|it| { - create_member_reference(it, self.ctxt.get_id_provider(), Some(base)) + needs_qualifier(flat_ref).map_or_else(Option::Some, |q| { + q.then_some("self") + .map(|it| create_member_reference(it, self.ctxt.get_id_provider(), None)) + .and_then(|base| { + initializer.get_flat_reference_name().map(|it| { + create_member_reference(it, self.ctxt.get_id_provider(), Some(base)) + }) }) - }) + }) }) } // we found a call-statement, must be `a : REF_TO ... := REF(b) | ADR(b)` - AstStatement::CallStatement(CallStatement { parameters, .. }) => parameters + AstStatement::CallStatement(CallStatement { operator, parameters }) => parameters .as_ref() .and_then(|it| it.as_ref().get_flat_reference_name()) .and_then(|flat_ref| { - needs_qualifier(flat_ref).then(|| { - create_call_statement( - "REF", - flat_ref, - Some("self"), - self.ctxt.id_provider.clone(), - &initializer.location, - ) + let op = operator.as_ref().get_flat_reference_name()?; + needs_qualifier(flat_ref).map_or_else(Option::Some, |q| { + q.then(|| { + create_call_statement( + op, + flat_ref, + Some("self"), + self.ctxt.id_provider.clone(), + &initializer.location, + ) + }) }) }), _ => return, diff --git a/src/validation/tests/variable_validation_tests.rs b/src/validation/tests/variable_validation_tests.rs index aff4257196..e87ea7248d 100644 --- a/src/validation/tests/variable_validation_tests.rs +++ b/src/validation/tests/variable_validation_tests.rs @@ -1246,3 +1246,66 @@ fn using_var_external_variable_without_matching_global_will_not_resolve() { "###); } + +#[test] +fn assigning_a_temp_reference_to_stateful_var_is_error() { + let diagnostics = parse_and_validate_buffered( + r#" + FUNCTION_BLOCK foo + VAR + s1: REF_TO DINT := REF(t1); // error + s2 AT t1 : DINT; // error + s3 : REFERENCE TO DINT REF= t1; // error + s4 AT s2 : DINT; // OK + s5 : REF_TO DINT := REF(s4); // OK + s6 : REFERENCE TO DINT REF= s5; // OK + END_VAR + VAR_TEMP + t1 : DINT; + t2 : REF_TO DINT := REF(t1); // OK + t3 AT s1 : DINT; // OK + t4 : REFERENCE TO DINT REF= t3; // OK + END_VAR + END_FUNCTION_BLOCK + + // all of these assignments are okay in a function, since they are all stack-variables + FUNCTION bar + VAR + s1: REF_TO DINT := REF(t1); + s2 AT t1 : DINT; + s3 : REFERENCE TO DINT REF= t1; + s4 AT s2 : DINT; + s5 : REF_TO DINT := REF(s4); + s6 : REFERENCE TO DINT REF= s5; + END_VAR + VAR_TEMP + t1 : DINT; + t2 : REF_TO DINT := REF(t1); + t3 AT s1 : DINT; + t4 : REFERENCE TO DINT REF= t3; + END_VAR + END_FUNCTION + "#, + ); + + assert_snapshot!(diagnostics, @r#" + error[E001]: Cannot assign address of temporary variable to a member-variable + ┌─ :4:40 + │ + 4 │ s1: REF_TO DINT := REF(t1); // error + │ ^^ Cannot assign address of temporary variable to a member-variable + + error[E001]: Cannot assign address of temporary variable to a member-variable + ┌─ :5:23 + │ + 5 │ s2 AT t1 : DINT; // error + │ ^^ Cannot assign address of temporary variable to a member-variable + + error[E001]: Cannot assign address of temporary variable to a member-variable + ┌─ :6:45 + │ + 6 │ s3 : REFERENCE TO DINT REF= t1; // error + │ ^^ Cannot assign address of temporary variable to a member-variable + + "#) +} diff --git a/src/validation/variable.rs b/src/validation/variable.rs index eb32c0269d..7158579d8b 100644 --- a/src/validation/variable.rs +++ b/src/validation/variable.rs @@ -1,18 +1,18 @@ use plc_ast::ast::{ - ArgumentProperty, ConfigVariable, Pou, PouType, Variable, VariableBlock, VariableBlockType, + ArgumentProperty, AstNode, AstStatement, CallStatement, ConfigVariable, Pou, PouType, Variable, + VariableBlock, VariableBlockType, }; use plc_diagnostics::diagnostics::Diagnostic; use super::{ array::validate_array_assignment, - statement::visit_statement, + statement::{validate_pointer_assignment, visit_statement}, types::{data_type_is_fb_or_class_instance, visit_data_type_declaration}, ValidationContext, Validator, Validators, }; -use crate::validation::statement::validate_enum_variant_assignment; -use crate::validation::statement::validate_pointer_assignment; use crate::{index::const_expressions::ConstExpression, resolver::AnnotationMap}; use crate::{index::const_expressions::UnresolvableKind, typesystem::DataTypeInformation}; +use crate::{index::PouIndexEntry, validation::statement::validate_enum_variant_assignment}; use crate::{index::VariableIndexEntry, resolver::StatementAnnotation}; pub fn visit_config_variable( @@ -301,6 +301,8 @@ fn validate_variable( return; }; + report_temporary_address_in_pointer_initializer(validator, context, v_entry, node); + validate_pointer_assignment( context, validator, @@ -353,6 +355,53 @@ fn validate_variable( } } +fn report_temporary_address_in_pointer_initializer( + validator: &mut Validator<'_>, + context: &ValidationContext<'_, T>, + v_entry: &VariableIndexEntry, + initializer: &AstNode, +) { + if v_entry.is_temp() { + return; + } + if let Some(pou) = context.qualifier.and_then(|q| context.index.find_pou(q)) { + match pou { + PouIndexEntry::Program { .. } + | PouIndexEntry::FunctionBlock { .. } + | PouIndexEntry::Class { .. } => (), + PouIndexEntry::Method { .. } => { + unimplemented!("We'll worry about this once we get around to OOP") + } + _ => return, + } + } + + let (Some(flat_ref), Some(location)) = (match &initializer.get_stmt() { + AstStatement::ReferenceExpr(_) => { + (initializer.get_flat_reference_name(), Some(initializer.get_location())) + } + AstStatement::CallStatement(CallStatement { parameters, .. }) => ( + parameters.as_ref().and_then(|it| it.as_ref().get_flat_reference_name()), + parameters.as_ref().map(|it| it.get_location()), + ), + _ => (None, None), + }) else { + return; + }; + + let Some(rhs_entry) = context.index.find_member(context.qualifier.unwrap_or_default(), flat_ref) else { + return; + }; + if !rhs_entry.is_temp() { + return; + } + + validator.diagnostics.push( + Diagnostic::new("Cannot assign address of temporary variable to a member-variable") + .with_location(location), + ); +} + /// Returns a diagnostic if a `REFERENCE TO` variable is incorrectly declared. fn validate_reference_to_declaration( validator: &mut Validator, @@ -364,7 +413,8 @@ fn validate_reference_to_declaration( return; }; - if !variable_ty.get_type_information().is_reference_to() && !variable_ty.get_type_information().is_alias() + if !(variable_ty.get_type_information().is_reference_to() + || variable_ty.get_type_information().is_alias()) { return; } @@ -387,8 +437,10 @@ fn validate_reference_to_declaration( } if let Some(ref initializer) = variable.initializer { - let type_lhs = context.index.find_type(inner_ty_name).unwrap(); - let type_rhs = context.annotations.get_type(initializer, context.index).unwrap(); + report_temporary_address_in_pointer_initializer(validator, context, variable_entry, initializer); + + let Some(type_lhs) = context.annotations.get_type_hint(initializer, context.index) else { return }; + let Some(type_rhs) = context.annotations.get_type(initializer, context.index) else { return }; validate_pointer_assignment(context, validator, type_lhs, type_rhs, &initializer.location); } diff --git a/tests/lit/single/init/function_locals.st b/tests/lit/single/init/function_locals.st index efa383ea55..a9022b166c 100644 --- a/tests/lit/single/init/function_locals.st +++ b/tests/lit/single/init/function_locals.st @@ -53,8 +53,35 @@ VAR END_VAR fb(); bb(); + baz(); printf('%d$N', temp_ref_to_temp^); // CHECK 42 printf('%d$N', temp_alias); // CHECK 42 printf('%d$N', temp_reference_to); // CHECK 42 +END_FUNCTION + +FUNCTION baz + VAR + s1: REF_TO LINT := REF(t1); + s2 AT t1 : LINT; + s3 : REFERENCE TO LINT REF= t1; + s4 AT s2 : LINT; + s5 : REF_TO LINT := REF(s4); + s6 : REFERENCE TO LINT REF= s4; + END_VAR + VAR_TEMP + t1 : LINT := 16#DEADBEEF; + t2 : REF_TO LINT := REF(t1); + t3 AT s2 : LINT; + t4 : REFERENCE TO LINT REF= s4; + END_VAR + printf('%#010x$N', s1^); // CHECK 0xdeadbeef + printf('%#010x$N', s2); // CHECK 0xdeadbeef + printf('%#010x$N', s3); // CHECK 0xdeadbeef + printf('%#010x$N', s4); // CHECK 0xdeadbeef + printf('%#010x$N', s5^); // CHECK 0xdeadbeef + printf('%#010x$N', s6); // CHECK 0xdeadbeef + printf('%#010x$N', t2^); // CHECK 0xdeadbeef + printf('%#010x$N', t3); // CHECK 0xdeadbeef + printf('%#010x$N', t4); // CHECK 0xdeadbeef END_FUNCTION \ No newline at end of file