Validate that memory access is done via variables

The address sanitizer is now able to detect whenever in an instruction a
memory access is done without using variables. This is now detected for
all instructions except for branching, which falls outside of this
scope.

Moreover, simple arithmetics is allowed and bounds are checked for
simple cases. That being said, more involved bound checks should be done
with other tools (e.g. emulators).

Signed-off-by: Miquel Sabaté Solà <mikisabate@gmail.com>

5 files changed, 204 insertions, 9 deletions

diff --git a/lib/xixanta/src/assembler.rs b/lib/xixanta/src/assembler.rs
index f63e785..09f0c9d 100644
--- a/lib/xixanta/src/assembler.rs
+++ b/lib/xixanta/src/assembler.rs
@@ -1,5 +1,8 @@
 use crate::mapping::{get_mapping_configuration, Mapping};
-use crate::node::{CommentType, ControlType, EchoKind, NodeType, OperationType, PNode, PString};
+use crate::node::{
+    is_asan_friendly_name, CommentType, ControlType, EchoKind, NodeType, OperationType, PNode,
+    PString,
+};
 use crate::object::{Bundle, Context, Object, ObjectType};
 use crate::opcodes::{AddressingMode, INSTRUCTIONS};
 use crate::parser::Parser;
@@ -856,9 +859,10 @@ impl<'a> Assembler<'a> {
                 }
 
                 // If it doesn't have the proper prefix, skip as well.
-                if !name.starts_with("zp_") && !name.starts_with("m_") && !name.starts_with("wr_") {
+                if !is_asan_friendly_name(name) {
                     continue;
                 }
+                let actual_name = name.split("::").last().unwrap_or("");
 
                 // Build up the memory range object for this bundle.
                 let val = bundle.bundle.value() as usize;
@@ -902,7 +906,7 @@ impl<'a> Assembler<'a> {
 
                 // Increase the counters for memory usage on either RAM slot and
                 // check for bounds.
-                if name.starts_with("zp_") || name.starts_with("m_") {
+                if actual_name.starts_with("zp_") || actual_name.starts_with("m_") {
                     memory.total_internal_ram += bundle.asan_reserve as usize;
 
                     if memory.total_internal_ram > 0x800 {
@@ -913,7 +917,7 @@ impl<'a> Assembler<'a> {
                             source: self.sources[0].clone(),
                         });
                     }
-                } else if name.starts_with("wr_") {
+                } else if actual_name.starts_with("wr_") {
                     memory.total_working_ram += bundle.asan_reserve as usize;
 
                     if memory.total_working_ram > 0x2000 {
@@ -2318,7 +2322,8 @@ impl<'a> Assembler<'a> {
                         });
                     }
 
-                    let val = self.evaluate_node(left.left.as_ref().unwrap())?;
+                    let evaluated_node = left.left.as_ref().unwrap();
+                    let val = self.evaluate_node(evaluated_node)?;
                     if val.size != 1 {
                         return Err(Error {
                             message: "address can only be one byte long on indirect Y addressing"
@@ -2328,6 +2333,7 @@ impl<'a> Assembler<'a> {
                             global: false,
                         });
                     }
+                    self.asan_check_arm(evaluated_node, &val)?;
                     return Ok((AddressingMode::IndirectY, val));
                 }
                 Err(Error {
@@ -2340,7 +2346,8 @@ impl<'a> Assembler<'a> {
             None => match left.right.as_ref() {
                 Some(right) => {
                     if right.value.value.trim().to_lowercase() == "x" {
-                        let val = self.evaluate_node(left.left.as_ref().unwrap())?;
+                        let evaluated_node = left.left.as_ref().unwrap();
+                        let val = self.evaluate_node(evaluated_node)?;
                         if val.size != 1 {
                             return Err(Error {
                                 message:
@@ -2351,6 +2358,7 @@ impl<'a> Assembler<'a> {
                                 global: false,
                             });
                         }
+                        self.asan_check_arm(evaluated_node, &val)?;
                         return Ok((AddressingMode::IndirectX, val));
                     }
                     Err(Error {
@@ -2361,7 +2369,8 @@ impl<'a> Assembler<'a> {
                     })
                 }
                 None => {
-                    let val = self.evaluate_node(left.left.as_ref().unwrap())?;
+                    let evaluated_node = left.left.as_ref().unwrap();
+                    let val = self.evaluate_node(evaluated_node)?;
                     if val.size != 2 {
                         return Err(Error {
                             message: "expecting a full 16-bit address".to_string(),
@@ -2370,6 +2379,7 @@ impl<'a> Assembler<'a> {
                             global: false,
                         });
                     }
+                    self.asan_check_arm(evaluated_node, &val)?;
                     Ok((AddressingMode::Indirect, val))
                 }
             },
@@ -2398,6 +2408,8 @@ impl<'a> Assembler<'a> {
         let right = node.right.as_ref().unwrap();
         match right.value.value.to_lowercase().trim() {
             "x" => {
+                self.asan_check_arm(left, &val)?;
+
                 // If the size == 2 but we can fit it on a single byte (i.e.
                 // because the second byte is just 0x00), then just "compress"
                 // this instruction. Note that this is only valid if the value
@@ -2429,6 +2441,8 @@ impl<'a> Assembler<'a> {
                 }
             }
             "y" => {
+                self.asan_check_arm(left, &val)?;
+
                 // Same optimization as with the "x" case.
                 if val.size == 1 || (val.resolved && val.bytes[1] == 0x00) {
                     // Similar to the case on "x" indexing.
@@ -2471,10 +2485,13 @@ impl<'a> Assembler<'a> {
             Some(LiteralMode::Hexadecimal) => {
                 // As for checking the most significant byte, it's the same
                 // optimization as with absolute to zeropage indexed addressing.
-                if base.is_branch()
-                    || val.size == 1
+                if base.is_branch() {
+                    val.size = 1;
+                    Ok((AddressingMode::RelativeOrZeropage, val))
+                } else if val.size == 1
                     || (val.bytes[1] == 0x00 && !matches!(base.value.value.as_str(), "jmp" | "jsr"))
                 {
+                    self.asan_check_arm(left_arm, &val)?;
                     val.size = 1;
                     Ok((AddressingMode::RelativeOrZeropage, val))
                 } else {
@@ -2508,6 +2525,107 @@ impl<'a> Assembler<'a> {
         }
     }
 
+    // Check the `node` considering is an instructions' "left" arm. More than
+    // that, it assumes that it's a memory access. Hence, only call this
+    // function if the addressing mode allows for it. Moreover, a `value` is
+    // also supplied to perform further checks on the given memory access.
+    //
+    // Note that it will only run if the address sanitizer is enabled, and it
+    // will check that any access only occurs as a variable or a simple pointer
+    // arithmetic. Also note that this function assumes that it's not being
+    // called by `jmp` or `jsr` kind of instructions on the absolute addressing
+    // mode, as they operate on another level.
+    fn asan_check_arm(&mut self, node: &PNode, value: &Bundle) -> Result<(), Error> {
+        if !self.asan_enabled {
+            return Ok(());
+        }
+
+        match node.node_type {
+            NodeType::Value => {
+                let name = &node.value.value;
+                if !is_asan_friendly_name(name) {
+                    // If it's referencing an actual ObjectType::Address, then
+                    // let it be (e.g. 'lda palettes, x'; where 'palettes' is a
+                    // legitimate name even if not 'is_asan_friendly_name').
+                    if let Ok(var) = self.context.get_variable(&node.value, &self.mappings) {
+                        if matches!(var.object_type, ObjectType::Address) {
+                            return Ok(());
+                        }
+                    }
+
+                    // Everything has been exhausted, this is actually not a
+                    // good name access.
+                    self.warnings.push(Error {
+                        line: node.value.line,
+                        message: format!(
+                            "accessing a memory region without a proper name ('{name}')"
+                        ),
+                        source: self.source_for(node),
+                        global: false,
+                    });
+                }
+                Ok(())
+            }
+            NodeType::Operation(OperationType::Add) | NodeType::Operation(OperationType::Sub) => {
+                // Get the name of the variable involved.
+                let left_name = &node.left.as_ref().unwrap().value.value;
+                let right_name = &node.left.as_ref().unwrap().value.value;
+                let name = if is_asan_friendly_name(left_name) {
+                    node.left.as_ref().unwrap()
+                } else if is_asan_friendly_name(right_name) {
+                    node.right.as_ref().unwrap()
+                } else {
+                    self.warnings.push(Error {
+                        line: node.value.line,
+                        message: "accessing a memory region without a proper name".to_string(),
+                        source: self.source_for(node),
+                        global: false,
+                    });
+                    return Ok(());
+                };
+
+                // Fetch the variable. If it doesn't exist or its value hasn't
+                // been resolved yet, we will ignore this check.
+                match self.context.get_variable(&name.value, &self.mappings) {
+                    Ok(object) => {
+                        // If it has a resolved value, then check that the
+                        // pointer arithmetics are within reserved bounds.
+                        if object.bundle.resolved {
+                            let original_value = object.bundle.value() as usize;
+                            let given_value = value.value() as usize;
+                            let end = original_value + object.asan_reserve as usize;
+
+                            if given_value < original_value || given_value >= end {
+                                let range = MemoryRange {
+                                    name: name.value.value.clone(),
+                                    range: (original_value..end),
+                                };
+                                return Err(Error {
+                                    line: node.value.line,
+                                    message: format!("out of bounds memory access for {range}"),
+                                    source: self.source_for(node),
+                                    global: false,
+                                });
+                            }
+                        }
+                        Ok(())
+                    }
+                    Err(_) => Ok(()),
+                }
+            }
+            _ => {
+                self.warnings.push(Error {
+                    line: node.value.line,
+                    message: "accessing a memory region without using a variable".to_string(),
+                    source: self.source_for(node),
+                    global: false,
+                });
+
+                Ok(())
+            }
+        }
+    }
+
     fn to_relative_address(&self, node: &PNode, bundle: &mut Bundle) -> Result<(), Error> {
         if !bundle.resolved {
             return Ok(());
diff --git a/lib/xixanta/src/node.rs b/lib/xixanta/src/node.rs
index 7cfc107..465dde2 100644
--- a/lib/xixanta/src/node.rs
+++ b/lib/xixanta/src/node.rs
@@ -1,5 +1,13 @@
 use std::fmt;
 
+/// Returns true if this String follows the naming
+/// conventions as expected by the address sanitizer.
+pub fn is_asan_friendly_name(string: &str) -> bool {
+    let name = string.split("::").last().unwrap_or("");
+
+    name.starts_with("zp_") || name.starts_with("wr_") || name.starts_with("m_")
+}
+
 /// A Positioned String. That is, a String which also has information on the
 /// line number and the column range.
 #[derive(Debug, Default, Clone, PartialEq)]
@@ -456,3 +464,25 @@ impl PNode {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn is_asan_friendly_name_test() {
+        let tests = vec![
+            ("zp_name", true),
+            ("name", false),
+            ("Scope::zp_name", true),
+            ("Scope::Inner::zp_good", true),
+            ("Scope::Inner::bad", false),
+        ];
+
+        for (name, expect) in tests {
+            let string = String::from(name);
+
+            assert_eq!(is_asan_friendly_name(&string), expect);
+        }
+    }
+}
diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh
index 9da3a31..ab7312b 100755
--- a/scripts/test-e2e.sh
+++ b/scripts/test-e2e.sh
@@ -52,9 +52,15 @@ exit_code=$((exit_code + $?))
 echo "test: custom => unused.nes"
 ./target/debug/nasm -c empty --asan -o tests/out/unused.nes tests/unused.s 2>tests/out/unused-warning.txt
 diff tests/out/unused-warning.txt tests/expected/unused-warning.txt
+exit_code=$((exit_code + $?))
 diff tests/out/unused.nes tests/expected/unused.nes
 exit_code=$((exit_code + $?))
 
+echo "test: custom => bare_accesses.nes"
+./target/debug/nasm -c empty --asan tests/bare_accesses.s 2>tests/out/bare_accesses.txt
+diff tests/out/bare_accesses.txt tests/expected/bare_accesses.txt
+exit_code=$((exit_code + $?))
+
 ##
 # code.nes
 
diff --git a/tests/bare_accesses.s b/tests/bare_accesses.s
new file mode 100644
index 0000000..42a6409
--- /dev/null
+++ b/tests/bare_accesses.s
@@ -0,0 +1,37 @@
+.segment "HEADER"
+    .byte 'N', 'E', 'S', $1A
+    .byte $02
+    .byte $01
+    .byte $00
+    .byte $00
+
+.segment "CODE"
+
+.scope Scope
+    zp_valid = $02
+
+    .scope Inner
+        zp_valid_too = $03
+    .endscope
+.endscope
+
+zp_used    = $00                ; asan:reserve $02
+whatever   = $01
+
+lda zp_used
+lda $00
+lda whatever
+lda Scope::zp_valid
+lda Scope::Inner::zp_valid_too
+lda zp_used + 1
+lda zp_used + 2
+lda zp_used - 1
+
+@something:
+    beq @something
+
+ldx #0
+lda palettes, x
+
+palettes:
+    .byte $0F
diff --git a/tests/expected/bare_accesses.txt b/tests/expected/bare_accesses.txt
new file mode 100644
index 0000000..4c14629
--- /dev/null
+++ b/tests/expected/bare_accesses.txt
@@ -0,0 +1,4 @@
+warning: accessing a memory region without using a variable (bare_accesses.s: line 22)
+warning: accessing a memory region without a proper name ('whatever') (bare_accesses.s: line 23)
+error: out of bounds memory access for 'zp_used' ($00-$01) (bare_accesses.s: line 27)
+error: out of bounds memory access for 'zp_used' ($00-$01) (bare_accesses.s: line 28)