Merge pull request diffblue#444 from diffblue/alfresco-fix-taint-lvsa-interface

SEC-445: Fix Alfresco failure to generate GOTO binary

Author: owen-jones-diffblue

benchmarks/TRAINING/OWASP/extract_benchmarks.py

@@ -79,7 +79,7 @@ def extract_benchmark(benchmark_name, java_bench_dir, html_files):
 
 def extract_benchmarks():
     if not os.path.exists(get_app_target_dir()):
-        print("ERROR: The directory '" + get_app_target_dir() + "' does not exists.")
+        print("ERROR: The directory '" + get_app_target_dir() + "' does not exist.")
         return
     html_files = collect_html_files()
     java_bench_dir = os.path.join(get_java_dir(), get_packages_relative_dir(), "testcode")
@@ -123,7 +123,7 @@ def main(cmdline):
         benchmark_pathname =\
             os.path.join(get_class_dir(), get_packages_relative_dir(), "testcode", cmdline.single + ".class")
         if not os.path.exists(benchmark_pathname):
-            print("ERROR: The benchmark file '" + benchmark_pathname + "' does not exists.")
+            print("ERROR: The benchmark file '" + benchmark_pathname + "' does not exist.")
             return
 
         extract_benchmark(

regression/december_demo_sprint/goto-analyzer/run.py

@@ -89,19 +89,19 @@ def  evaluate_one_directory(cmdline):
         print("ERROR: Directory containing specification files was not specified.")
         return
     if not os.path.exists(cmdline.spec_dir) or os.path.isfile(cmdline.spec_dir):
-        print("ERROR: Directory containing specification files does not exists: " + cmdline.spec_dir)
+        print("ERROR: Directory containing specification files does not exist: " + cmdline.spec_dir)
         return
     if cmdline.sources_dir is None:
         print("ERROR: Sources dir of the analysed Java web application was not specified.")
         return
     if not os.path.exists(cmdline.sources_dir) or os.path.isfile(cmdline.sources_dir):
-        print("ERROR: Sources dir of the analysed Java web application does not exists: " + cmdline.sources_dir)
+        print("ERROR: Sources dir of the analysed Java web application does not exist: " + cmdline.sources_dir)
         return
     if cmdline.binaries_dir is None:
         print("ERROR: Binaries dir of the analysed Java web application was not specified.")
         return
     if not os.path.exists(cmdline.binaries_dir) or os.path.isfile(cmdline.binaries_dir):
-        print("ERROR: Binaries dir of the analysed Java web application does not exists: " + cmdline.binaries_dir)
+        print("ERROR: Binaries dir of the analysed Java web application does not exist: " + cmdline.binaries_dir)
         return
     if cmdline.temp_dir is None:
         print("ERROR: Temp dir of the analyser was not specified.")

src/pointer-analysis/local_value_set.cpp

@@ -85,8 +85,30 @@ upcast_to_type(exprt expr, const typet &intended_type, const namespacet &ns)
   return expr;
 }
 
-/// Remove all superclass accesses from `expr` and return the result
-static exprt remove_all_typecasts_and_superclass_accesses(exprt expr)
+/// Check whether the passed expression represents an upcast to a parent.
+/// \param expr: The examined expression
+/// \param ns: A namespace; the 'follow' method is needed
+/// \return True, if 'expr' is an upcast, and false otherwise.
+bool is_parent_member_access(const member_exprt &expr, const namespacet &ns)
+{
+  const typet &parent_type = ns.follow(expr.type());
+  if(parent_type.id() != ID_struct)
+    return false;
+  const typet &child_type = ns.follow(expr.compound().type());
+  INVARIANT(
+    child_type.id() == ID_struct,
+    "The compound of a member_exprt should be a struct");
+  optionalt<typet> base_type = get_base_type(child_type, ns);
+  return base_type && *base_type == parent_type;
+}
+
+/// Remove all superclass accesses from `expr`
+/// \param expr: input expression
+/// \param ns: namespace
+/// \return input expression with all typecasts and superclass accesses removed
+exprt remove_all_typecasts_and_superclass_accesses(
+  exprt expr,
+  const namespacet &ns)
 {
   while(expr.id() == ID_member || expr.id() == ID_typecast)
   {
@@ -96,20 +118,36 @@ static exprt remove_all_typecasts_and_superclass_accesses(exprt expr)
     }
     else
     {
-      const member_exprt &member = to_member_expr(expr);
-      const std::string &component_name =
-        id2string(member.get_component_name());
-      const bool is_superclass_access = has_prefix(component_name, "@") &&
-                                        component_name != "@lock" &&
-                                        component_name != "@class_identifier";
-      if(!is_superclass_access)
+      const member_exprt &member = expr_checked_cast<member_exprt>(expr);
+      if(!is_parent_member_access(member, ns))
         break;
       expr = member.compound();
     }
   }
   return expr;
 }
 
+/// Remove any typecasts or soft casts from `input` and return the result, where
+/// a "soft cast" means dereferencing a pointer to a class, accessing the field
+/// corresponding to its base class, and taking the address of that field -
+/// `&input->parent_class`
+/// \param input: input expression
+/// \param ns: namespace
+/// \return input expression with all typecasts and soft casts removed
+exprt remove_casts(exprt input, const namespacet &ns)
+{
+  while(input.id() == ID_typecast)
+    input = input.op0();
+  if(input.id() == ID_address_of)
+  {
+    const exprt &input_target = to_address_of_expr(input).object();
+    exprt expr = remove_all_typecasts_and_superclass_accesses(input_target, ns);
+    if(expr != input_target && expr.id() == ID_dereference)
+      return to_dereference_expr(expr).pointer();
+  }
+  return input;
+}
+
 void local_value_sett::make_union_adjusting_types(
   object_mapt &dest,
   const object_mapt &src,
@@ -120,7 +158,8 @@ void local_value_sett::make_union_adjusting_types(
   {
     // Remove soft-casts (and hard casts?) from the rhs value
     const exprt underlying_object =
-      remove_all_typecasts_and_superclass_accesses(object_numbering[num.first]);
+      remove_all_typecasts_and_superclass_accesses(
+        object_numbering[num.first], ns);
 
     if(underlying_object.id() == ID_external_value_set)
     {
@@ -273,24 +312,6 @@ static const typet &type_from_suffix(
   return *ret;
 }
 
-/// Remove any soft casts from `input` and return the result, where a "soft
-/// cast" means dereferencing a pointer to a class, accessing the field
-/// corresponding to its base class, and taking the address of that field -
-/// `&input->parent_class`
-static exprt remove_casts(exprt input)
-{
-  while(input.id() == ID_typecast)
-    input = input.op0();
-  if(input.id() == ID_address_of)
-  {
-    const exprt &input_target = to_address_of_expr(input).object();
-    exprt expr = remove_all_typecasts_and_superclass_accesses(input_target);
-    if(expr != input_target && expr.id() == ID_dereference)
-      return to_dereference_expr(expr).pointer();
-  }
-  return input;
-}
-
 void local_value_sett::get_value_set_rec(
   const exprt &expr,
   object_mapt &dest,
@@ -310,7 +331,7 @@ void local_value_sett::get_value_set_rec(
       object_mapt tmp;
       // Hard casts have been removed by the simplifier but soft casts need
       // to be removed too (maybe?)
-      exprt expr_to_cast = remove_casts(typecast_expr.op());
+      exprt expr_to_cast = remove_casts(typecast_expr.op(), ns);
       get_value_set_rec(expr_to_cast, tmp, suffix, original_type, ns);
       make_union_adjusting_types(dest, tmp, typecast_expr.type().subtype(), ns);
     }

src/pointer-analysis/local_value_set.h

@@ -169,5 +169,9 @@ class local_value_sett : public value_sett
 /// \return the name of the base type
 const irep_idt &
 update_expr_with_soft_cast_to_base_type(exprt &expr, const namespacet &ns);
-
+bool is_parent_member_access(const member_exprt &expr, const namespacet &ns);
+exprt remove_all_typecasts_and_superclass_accesses(
+  exprt expr,
+  const namespacet &ns);
+exprt remove_casts(exprt input, const namespacet &ns);
 #endif

src/taint-analysis/taint_summary.cpp

@@ -162,38 +162,39 @@ static exprt transform_external_objects(const exprt& e)
   return e;
 }
 
+/// Use the output of LVSA to create a set of expressions which `query` might
+/// point to at a given point in the program. String literals, null objects and
+/// precise access path external values sets are not included in the output.
+/// \param query: input expression
+/// \param ns: namespace
+/// \param lvsa: the output of LVSA, to work out what `query` might point to
+/// \param instit: the point in the program to do the query
+/// \param [out] result: output
 static void collect_lvsa_access_paths(
-  exprt const& query_in,
-  namespacet const& ns,
-  local_value_set_analysist& lvsa,
-  instruction_iteratort const& instit,
+  exprt const &query,
+  namespacet const &ns,
+  local_value_set_analysist &lvsa,
+  instruction_iteratort const &instit,
   std::set<exprt> &result)
 {
   TMPROF_BLOCK();
 
-  if(query_in.id()==ID_side_effect)
+  if(query.id() == ID_side_effect)
   {
-    side_effect_exprt const see=to_side_effect_expr(query_in);
+    side_effect_exprt const &see = to_side_effect_expr(query);
     if(see.get_statement()==ID_nondet)
     {
       // TODO(marek-trtik): Add computation of a proper points-to set for NONDET
       //                    in a side-effect expression
-      assert(false);
     }
   }
 
-  const exprt* query=&query_in;
-  while(query->id()==ID_typecast)
-    query=&query->op0();
-  const exprt& e = *query;
-
-  if(e.id()==ID_symbol ||
-     e.id()==ID_index ||
-     e.id()==ID_member ||
-     e.id()==ID_dereference)
+  if(
+    query.id() == ID_symbol || query.id() == ID_index ||
+    query.id() == ID_member || query.id() == ID_dereference)
   {
     value_setst::valuest referees;
-    lvsa.get_values_reconstructed(instit,address_of_exprt(e),referees);
+    lvsa.get_values_reconstructed(instit, address_of_exprt(query), referees);
     for(const auto& target : referees)
     {
       if(target.id()==ID_unknown)
@@ -230,7 +231,7 @@ static void collect_lvsa_access_paths(
   }
   else
   {
-    forall_operands(it,e)
+    forall_operands(it, query)
       collect_lvsa_access_paths(
         *it,
         ns,
@@ -240,6 +241,8 @@ static void collect_lvsa_access_paths(
   }
 }
 
+/// Call `collect_lvsa_access_paths` and use taint_object_numbering to convert
+/// the results from `exprt`s to the format required for `lvalue_numbers_sett`
 static void collect_lvsa_access_paths(
   exprt const& query_in,
   namespacet const& ns,
@@ -361,52 +364,20 @@ exprt dereference_if_pointer(exprt expr)
   return expr;
 }
 
-/// \brief Checks whether the passed expression represents an up cast to a
-/// parent.
-/// \param expr: The examined expression.
-/// \param hierarchy: The class heierarchy used for checking parent-child
-/// relation.
-/// \param ns: A namespace; the 'follow' method is needed.
-/// \return True, if 'expr' is an up cast, and false otherwise.
-bool is_parent_member_access(
-  const member_exprt &expr,
-  const class_hierarchyt &hierarchy,
-  const namespacet &ns)
-{
-  if(expr.get_component_number() != 0UL)
-    return false;
-  const typet &parent_type = ns.follow(expr.type());
-  if(parent_type.id() != ID_struct)
-    return false;
-  const typet &child_type = ns.follow(expr.compound().type());
-  INVARIANT(child_type.id() == ID_struct, "Compound of member_exprt is struct");
-  const std::string parent_name =
-    "java::" + as_string(to_struct_type(parent_type).get_tag());
-  const std::string child_name =
-    "java::" + as_string(to_struct_type(child_type).get_tag());
-  const auto it = hierarchy.class_map.find(parent_name);
-  if(it == hierarchy.class_map.cend())
-    return false;
-  for(const auto &child : it->second.children)
-    if(child == child_name)
-      return true;
-  return false;
-}
-
 /// \brief An extended version of the original LVSA utility function
 /// 'collect_lvsa_access_paths'. Namely, when the original function
 /// returns an empty set, then the extended one introduces a fresh
-/// EVS object for the passed (and dereferenced) expression. For non-empy
+/// EVS object for the passed (and dereferenced) expression. For non-empty
 /// output from the original function it iterates over all retrieved
 /// numbers and for each number representing an upcast (member_exprt) it calls
 /// itself in order to retrieve numbers also to parent accesses. This is because
 /// an upcast is represented in LVSA by one number and we also need LVSA
 /// number(s) of the actual parent object.
-/// \param expr: The expression whose dereference is passed to LVSA.
-/// \param it: The current program location (instruction iteratior)
+/// \param expr: The expression to look up in LVSA.
+/// \param it: The current program location (instruction iterator)
 /// \param lvsa: The LVSA analysis.
 /// \param result: The resulting points-to set.
-/// \return True, if the resulting points-to set is definitelly singular, and
+/// \return True, if the resulting points-to set is definitely singular, and
 /// false otherwise.
 bool taint_algorithm_computing_summary_of_functiont::
   collect_lvsa_access_paths_extended(
@@ -419,7 +390,8 @@ bool taint_algorithm_computing_summary_of_functiont::
   TMPROF_BLOCK();
 
   bool singular = false;
-  const exprt query_expr = dereference_if_pointer(expr);
+  const exprt query_expr =
+    dereference_if_pointer(remove_casts(expr, program->get_namespace()));
   lvalue_numbers_sett raw_result;
   collect_lvsa_access_paths(
     query_expr,
@@ -454,11 +426,7 @@ bool taint_algorithm_computing_summary_of_functiont::
     const exprt &raw_expr = numbering->at(raw_number);
     if(auto member_expr = expr_try_dynamic_cast<member_exprt>(raw_expr))
     {
-      if(
-        is_parent_member_access(
-          *member_expr,
-          program->get_class_hierarchy(),
-          program->get_namespace()))
+      if(is_parent_member_access(*member_expr, program->get_namespace()))
       {
         if(
           !collect_lvsa_access_paths_extended(
@@ -1202,7 +1170,7 @@ numbered_lvalue_to_taint_mapt taint_algorithm_computing_summary_of_functiont::
                  std::to_string(propagation_rule.get_id()),
                  "Although a propagation rule " +
                    std::to_string(propagation_rule.get_id()) +
-                   "was found, it was NOT applied, because the symbolic value "
+                   " was found, it was NOT applied, because the symbolic value "
                    "to be assigned is actually BOTTOM."});
             }
             else