add runtime sanity checks

In contrast to most of the other parts of the codebase, the "sexpr" utility and manipulation code deals with a lot of raw pointers. This change adds a number of runtime assertions and null pointer checks so that potential future bugs in consumers of the API will trigger an exception instead of generating invalid memory reads or writes. Still, correct could should never trigger any of the newly added runtime errors; every thrown exception is considered a bug.
asmuth · May 24, 2020 · 078f820 · 078f820
1 parent 4a24ae3
commit 078f820
Show file tree

Hide file tree

Showing 8 changed files with 104 additions and 18 deletions.
diff --git a/src/color_reader.cc b/src/color_reader.cc
@@ -168,6 +168,10 @@ ReturnCode color_map_read_gradient(
     }
 
     gradient.emplace_back(step, color);
+
+    if (!expr_has_next(expr)) {
+      break;
+    }
   }
 
   *color_map = color_map_gradient(gradient);
@@ -207,6 +211,10 @@ ReturnCode color_map_read_steps(
     }
 
     steps.emplace_back(step, color);
+
+    if (!expr_has_next(expr)) {
+      break;
+    }
   }
 
   *color_map = color_map_steps(steps);

diff --git a/src/eval.cc b/src/eval.cc
@@ -66,6 +66,10 @@ ReturnCode eval(
     if (auto rc = cmd->fn(ctx, args); !rc) {
       return rc;
     }
+
+    if (!expr_has_next(expr)) {
+      break;
+    }
   }
 
   return OK;

diff --git a/src/sexpr.cc b/src/sexpr.cc
@@ -34,6 +34,10 @@ struct Expr {
 };
 
 void expr_destroy(Expr* expr) {
+  if (!expr) {
+    throw std::runtime_error("invalid expression");
+  }
+
   delete expr;
 }
 
@@ -65,18 +69,42 @@ ExprStorage expr_create_value_literal(const std::string& str) {
 }
 
 Expr* expr_next(Expr* expr) {
+  if (!expr) {
+    throw std::runtime_error("invalid expression");
+  }
+
   return expr->next.get();
 }
 
 const Expr* expr_next(const Expr* expr) {
+  if (!expr) {
+    throw std::runtime_error("invalid expression");
+  }
+
   return expr->next.get();
 }
 
+bool expr_has_next(const Expr* expr) {
+  if (!expr) {
+    throw std::runtime_error("invalid expression");
+  }
+
+  return !!expr->next;
+}
+
 void expr_set_next(Expr* expr, ExprStorage next) {
+  if (!expr) {
+    throw std::runtime_error("invalid expression");
+  }
+
   expr->next = std::move(next);
 }
 
 ExprStorage* expr_get_next_storage(Expr* expr) {
+  if (!expr) {
+    throw std::runtime_error("invalid expression");
+  }
+
   return &expr->next;
 }
 
@@ -89,10 +117,18 @@ bool expr_is_list(const Expr* expr, const std::string& head) {
 }
 
 const Expr* expr_get_list(const Expr* expr) {
+  if (!expr || expr->type != ExprType::LIST) {
+    throw std::runtime_error("invalid expression (not a list)");
+  }
+
   return expr->list.get();
 }
 
 const Expr* expr_get_list_tail(const Expr* expr) {
+  if (!expr || expr->type != ExprType::LIST) {
+    throw std::runtime_error("invalid expression (not a list)");
+  }
+
   if (auto l = expr->list.get(); l) {
     return expr_next(l);
   } else {
@@ -101,6 +137,10 @@ const Expr* expr_get_list_tail(const Expr* expr) {
 }
 
 ExprStorage* expr_get_list_storage(Expr* expr) {
+  if (!expr) {
+    throw std::runtime_error("invalid expression");
+  }
+
   return &expr->list;
 }
 
@@ -129,6 +169,10 @@ bool expr_is_value_quoted(const Expr* expr, const std::string& cmp) {
 }
 
 const std::string& expr_get_value(const Expr* expr) {
+  if (!expr) {
+    throw std::runtime_error("invalid expression");
+  }
+
   return expr->value;
 }
 
@@ -153,8 +197,11 @@ ExprStorage expr_clone(const Expr* e, int count /* =-1 */) {
 }
 
 std::string expr_inspect(const Expr* expr) {
-  std::stringstream s;
+  if (!expr) {
+    return "<null>";
+  }
 
+  std::stringstream s;
   switch (expr->type) {
     case ExprType::VALUE:
     case ExprType::VALUE_LITERAL:

diff --git a/src/sexpr.h b/src/sexpr.h
@@ -28,6 +28,7 @@ ExprStorage expr_create_value_literal(const std::string& str);
 
 Expr* expr_next(Expr* expr);
 const Expr* expr_next(const Expr* expr);
+bool expr_has_next(const Expr* expr);
 void expr_set_next(Expr* expr, ExprStorage next);
 ExprStorage* expr_get_next_storage(Expr* expr);
 

diff --git a/src/sexpr_conv.cc b/src/sexpr_conv.cc
@@ -99,7 +99,7 @@ ReturnCode expr_to_float64_opt_pair(
     const Expr* expr,
     Option<double>* v1,
     Option<double>* v2) {
-  if (expr  && expr_is_list(expr)) {
+  if (expr && expr_is_list(expr)) {
     expr = expr_get_list(expr);
   } else {
     return errorf(
@@ -108,19 +108,18 @@ ReturnCode expr_to_float64_opt_pair(
         expr_inspect(expr));
   }
 
-  for (size_t i = 0; i < 2; ++i) {
-    if (!expr || !expr_is_value(expr)) {
-      return errorf(
-          ERROR,
-          "argument error; expected a value, got: {}",
-          expr_inspect(expr));
-    }
+  const auto& args = expr_collect(expr);
 
-    if (auto rc = expr_to_float64_opt(expr, i == 0 ? v1 : v2); !rc) {
-      return rc;
-    }
+  if (args.size() != 2) {
+    return err_invalid_nargs(args.size(), 2);
+  }
 
-    expr = expr_next(expr);
+  if (auto rc = expr_to_float64_opt(args[0], v1); !rc) {
+    return rc;
+  }
+
+  if (auto rc = expr_to_float64_opt(args[1], v2); !rc) {
+    return rc;
   }
 
   return OK;

diff --git a/src/sexpr_conv_impl.h b/src/sexpr_conv_impl.h
@@ -90,6 +90,10 @@ ReturnCode expr_tov_flat(
     }
 
     values->emplace_back(std::move(v));
+
+    if (!expr_has_next(expr)) {
+      break;
+    }
   }
 
   return OK;

diff --git a/src/sexpr_util.cc b/src/sexpr_util.cc
@@ -22,6 +22,10 @@ void expr_each(
     std::function<void (const Expr* e)> fn) {
   for (; expr; expr = expr_next(expr)) {
     fn(expr);
+
+    if (!expr_has_next(expr)) {
+      break;
+    }
   }
 }
 
@@ -51,6 +55,10 @@ ReturnCode expr_walk_map(
         return rc;
       }
     }
+
+    if (!expr_has_next(expr)) {
+      break;
+    }
   }
 
   return OK;
@@ -60,7 +68,7 @@ ReturnCode expr_walk_map_wrapped(
     const Expr* expr,
     const std::unordered_map<std::string, ExprVisitor>& fns,
     bool strict /* = true */) {
-  if (!expr_is_list(expr)) {
+  if (!expr || !expr_is_list(expr)) {
     return error(ERROR, "expected a list");
   }
 
@@ -94,6 +102,10 @@ ReturnCode expr_walk_tmap(
         return rc;
       }
     }
+
+    if (!expr_has_next(expr)) {
+      break;
+    }
   }
 
   return OK;
@@ -120,6 +132,11 @@ std::vector<const Expr*> expr_collect(const Expr* expr) {
 
   while (expr) {
     exprs.push_back(expr);
+
+    if (!expr_has_next(expr)) {
+      break;
+    }
+
     expr = expr_next(expr);
   }
 

diff --git a/src/style_reader.cc b/src/style_reader.cc
@@ -58,8 +58,11 @@ ReturnCode stroke_style_read(
   }
 
   if (expr_is_value(expr, "dash") ||
-      expr_is_value(expr, "dashed") ||
-      expr_is_list(expr, "dash") ||
+      expr_is_value(expr, "dashed")) {
+    return stroke_style_read_dash(ctx, nullptr, style);
+  }
+
+  if (expr_is_list(expr, "dash") ||
       expr_is_list(expr, "dashed")) {
     return stroke_style_read_dash(ctx, expr_get_list_tail(expr), style);
   }
@@ -130,8 +133,11 @@ ReturnCode fill_style_read(
   }
 
   if (expr_is_value(expr, "hatch") ||
-      expr_is_value(expr, "hatched") ||
-      expr_is_list(expr, "hatch") ||
+      expr_is_value(expr, "hatched")) {
+    return fill_style_read_hatch(ctx, nullptr, style);
+  }
+
+  if (expr_is_list(expr, "hatch") ||
       expr_is_list(expr, "hatched")) {
     return fill_style_read_hatch(ctx, expr_get_list_tail(expr), style);
   }
-Original file line number
+Diff line change
@@ Expand Up / @@ -90,6 +90,10 @@ ReturnCode expr_tov_flat( @@
         }
         values->emplace_back(std::move(v));
+        if (!expr_has_next(expr)) {
+          break;
+        }
       }
       return OK;
@@ Expand Down @@