From dbee63338fda39170118e3ab1f66637a690f5542 Mon Sep 17 00:00:00 2001
From: Miquel Sabaté Solà <mikisabate@gmail.com>
Date: Wed, 18 Dec 2024 11:55:40 +0100
Subject: Prevent a crash on bad binary literals
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When evaluating binary literals we allowed the shift value to grow as
needed and we checked whether it was a good value after evaluating the
literal. This is bad for performance reasons: if we are expecting an
exact size for a literal (8 digits here), do not even attempt to do
anything at all if the size doesn't match.

Moreover, in some extreme cases this could result into an overflow of
the 'shift' variable, which was promptly catched by Rust's bound
checker.

Signed-off-by: Miquel Sabaté Solà <mikisabate@gmail.com>
---
 lib/xixanta/src/assembler.rs | 60 ++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

(limited to 'lib/xixanta/src/assembler.rs')

diff --git a/lib/xixanta/src/assembler.rs b/lib/xixanta/src/assembler.rs
index 24653e2..f2cd507 100644
--- a/lib/xixanta/src/assembler.rs
+++ b/lib/xixanta/src/assembler.rs
@@ -601,9 +601,29 @@ impl Assembler {
     fn evaluate_binary(&mut self, node: &PNode) -> Result<Bundle, EvalError> {
         let string = node.value.value.as_str();
         let mut value = 0;
-        let mut shift = 0;
 
-        for c in string.chars().rev() {
+        // We are strict with the definition of binary values to avoid gotchas.
+        // Force the literal to be exactly 8 digits wide. If that's not the
+        // case, do not even try evaluating it.
+        match string.len().cmp(&8) {
+            Ordering::Less => {
+                return Err(EvalError {
+                    message: "missing binary digits to get a full byte".to_string(),
+                    line: node.value.line,
+                    global: false,
+                })
+            }
+            Ordering::Greater => {
+                return Err(EvalError {
+                    message: "too many binary digits for a single byte".to_string(),
+                    line: node.value.line,
+                    global: false,
+                })
+            }
+            _ => {}
+        }
+
+        for (shift, c) in string.chars().rev().enumerate() {
             if c == '1' {
                 let val = 1 << shift;
                 value += val;
@@ -624,30 +644,16 @@ impl Assembler {
                     global: false,
                 });
             }
-
-            shift += 1;
         }
 
-        match shift.cmp(&8) {
-            Ordering::Less => Err(EvalError {
-                message: "missing binary digits to get a full byte".to_string(),
-                line: node.value.line,
-                global: false,
-            }),
-            Ordering::Greater => Err(EvalError {
-                message: "too many binary digits for a single byte".to_string(),
-                line: node.value.line,
-                global: false,
-            }),
-            Ordering::Equal => Ok(Bundle {
-                bytes: [value as u8, 0, 0],
-                size: 1,
-                address: 0,
-                cycles: 0,
-                affected_on_page: false,
-                resolved: true,
-            }),
-        }
+        Ok(Bundle {
+            bytes: [value as u8, 0, 0],
+            size: 1,
+            address: 0,
+            cycles: 0,
+            affected_on_page: false,
+            resolved: true,
+        })
     }
 
     fn evaluate_decimal(&mut self, node: &PNode) -> Result<Bundle, EvalError> {
@@ -1373,9 +1379,9 @@ mod tests {
         );
         assert_error(
             r#"
-Variable = 42
-adc %Variable
-"#,
+        Variable = 42
+        adc %Variable
+        "#,
             "Evaluation",
             3,
             false,
-- 
cgit v1.2.3