Clean up handling of temporary files in test suite (lsils#652)

* Don't write `test.aig` from `write_aiger` test

I assume this was added for debugging purposes at some point.
The test doesn't need it and the current working directory might not be
writable.

* Set the current working directory to a temporary directory when running tests

The incoming current working directory may not be writable.

* Propagate I/O errors when (de)serializing

This is especially important when deserializing. If we don't catch
errors after reading `size`, we may use its uninitialized value as a
loop bound, which is very bad.

* Add tests for I/O error propagation

Author: rocallahan

include/mockturtle/io/serialize.hpp

@@ -42,6 +42,7 @@
 
 #include "../networks/aig.hpp"
 #include <fstream>
+#include <optional>
 #include <parallel_hashmap/phmap_dump.h>
 
 namespace mockturtle
@@ -93,7 +94,10 @@ struct serializer
   bool operator()( phmap::BinaryOutputArchive& os, regular_node<Fanin, Size, PointerFieldSize> const& n ) const
   {
     uint64_t size = n.children.size();
-    os.dump( (char*)&size, sizeof( uint64_t ) );
+    if ( !os.dump( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
 
     for ( const auto& c : n.children )
     {
@@ -105,7 +109,10 @@ struct serializer
     }
 
     size = n.data.size();
-    os.dump( (char*)&size, sizeof( uint64_t ) );
+    if ( !os.dump( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
     for ( const auto& d : n.data )
     {
       bool result = this->operator()( os, d );
@@ -122,7 +129,11 @@ struct serializer
   bool operator()( phmap::BinaryInputArchive& ar_input, const regular_node<Fanin, Size, PointerFieldSize>* n ) const
   {
     uint64_t size;
-    ar_input.load( (char*)&size, sizeof( uint64_t ) );
+    if ( !ar_input.load( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
+
     for ( uint64_t i = 0; i < size; ++i )
     {
       pointer_type ptr;
@@ -163,7 +174,10 @@ struct serializer
   {
     /* nodes */
     uint64_t size = storage.nodes.size();
-    os.dump( (char*)&size, sizeof( uint64_t ) );
+    if ( !os.dump( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
     for ( const auto& n : storage.nodes )
     {
       if ( !this->operator()( os, n ) )
@@ -174,7 +188,10 @@ struct serializer
 
     /* inputs */
     size = storage.inputs.size();
-    os.dump( (char*)&size, sizeof( uint64_t ) );
+    if ( !os.dump( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
     for ( const auto& i : storage.inputs )
     {
       if ( !this->operator()( os, i ) )
@@ -185,7 +202,10 @@ struct serializer
 
     /* outputs */
     size = storage.outputs.size();
-    os.dump( (char*)&size, sizeof( uint64_t ) );
+    if ( !os.dump( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
     for ( const auto& o : storage.outputs )
     {
       if ( !this->operator()( os, o ) )
@@ -200,7 +220,10 @@ struct serializer
       return false;
     }
 
-    os.dump( (char*)&storage.trav_id, sizeof( uint32_t ) );
+    if ( !os.dump( (char*)&storage.trav_id, sizeof( uint32_t ) ) )
+    {
+      return false;
+    }
 
     return true;
   }
@@ -209,7 +232,10 @@ struct serializer
   {
     /* nodes */
     uint64_t size;
-    ar_input.load( (char*)&size, sizeof( uint64_t ) );
+    if ( !ar_input.load( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
     for ( uint64_t i = 0; i < size; ++i )
     {
       node_type n;
@@ -221,16 +247,25 @@ struct serializer
     }
 
     /* inputs */
-    ar_input.load( (char*)&size, sizeof( uint64_t ) );
+    if ( !ar_input.load( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
     for ( uint64_t i = 0; i < size; ++i )
     {
       uint64_t value;
-      ar_input.load( (char*)&value, sizeof( uint64_t ) );
+      if ( !ar_input.load( (char*)&value, sizeof( uint64_t ) ) )
+      {
+        return false;
+      }
       storage->inputs.push_back( value );
     }
 
     /* outputs */
-    ar_input.load( (char*)&size, sizeof( uint64_t ) );
+    if ( !ar_input.load( (char*)&size, sizeof( uint64_t ) ) )
+    {
+      return false;
+    }
     for ( uint64_t i = 0; i < size; ++i )
     {
       pointer_type ptr;
@@ -247,23 +282,36 @@ struct serializer
       return false;
     }
 
-    ar_input.load( (char*)&storage->trav_id, sizeof( uint32_t ) );
+    if ( !ar_input.load( (char*)&storage->trav_id, sizeof( uint32_t ) ) )
+    {
+      return false;
+    }
 
     return true;
   }
 }; /* struct serializer */
 
 } /* namespace detail */
 
+/*! \brief Serializes a combinational AIG network to a archive, returning false on failure
+ *
+ * \param aig Combinational AIG network
+ * \param os Output archive
+ */
+inline bool serialize_network_fallible( aig_network const& aig, phmap::BinaryOutputArchive& os )
+{
+  detail::serializer _serializer;
+  return _serializer( os, *aig._storage );
+}
+
 /*! \brief Serializes a combinational AIG network to a archive
  *
  * \param aig Combinational AIG network
  * \param os Output archive
  */
 inline void serialize_network( aig_network const& aig, phmap::BinaryOutputArchive& os )
 {
-  detail::serializer _serializer;
-  bool const okay = _serializer( os, *aig._storage );
+  bool const okay = serialize_network_fallible( aig, os );
   (void)okay;
   assert( okay && "failed to serialize the network onto stream" );
 }
@@ -279,12 +327,12 @@ inline void serialize_network( aig_network const& aig, std::string const& filena
   serialize_network( aig, ar_out );
 }
 
-/*! \brief Deserializes a combinational AIG network from a input archive
+/*! \brief Deserializes a combinational AIG network from a input archive, returning nullopt on failure
  *
  * \param ar_input Input archive
  * \return Deserialized AIG network
  */
-inline aig_network deserialize_network( phmap::BinaryInputArchive& ar_input )
+inline std::optional<aig_network> deserialize_network_fallible( phmap::BinaryInputArchive& ar_input )
 {
   detail::serializer _serializer;
   auto storage = std::make_shared<aig_storage>();
@@ -293,10 +341,25 @@ inline aig_network deserialize_network( phmap::BinaryInputArchive& ar_input )
   storage->outputs.clear();
   storage->hash.clear();
 
-  bool const okay = _serializer( ar_input, storage.get() );
-  (void)okay;
-  assert( okay && "failed to deserialize the network onto stream" );
-  return aig_network{ storage };
+  if ( _serializer( ar_input, storage.get() ) )
+  {
+    return aig_network{ storage };
+  }
+
+  return std::nullopt;
+}
+
+/*! \brief Deserializes a combinational AIG network from a input archive
+ *
+ * \param ar_input Input archive
+ * \return Deserialized AIG network
+ */
+inline aig_network deserialize_network( phmap::BinaryInputArchive& ar_input )
+{
+  auto result = deserialize_network_fallible( ar_input );
+  (void)result.has_value();
+  assert( result.has_value() && "failed to deserialize the network onto stream" );
+  return *result;
 }
 
 /*! \brief Deserializes a combinational AIG network from a file

lib/parallel_hashmap/parallel_hashmap/phmap_dump.h

@@ -21,6 +21,7 @@
 
 #include <iostream>
 #include <fstream>
+#include <limits>
 #include <sstream>
 #include "phmap.h"
 namespace phmap
@@ -188,47 +189,66 @@ bool parallel_hash_set<N, RefSet, Mtx_, Policy, Hash, Eq, Alloc>::load(InputArch
 // ------------------------------------------------------------------------
 class BinaryOutputArchive {
 public:
-    BinaryOutputArchive(const char *file_path) {
+    BinaryOutputArchive(const char *file_path,
+                        size_t bytes_remaining = std::numeric_limits<size_t>::max())
+        : bytes_remaining_(bytes_remaining) {
         ofs_.open(file_path, std::ios_base::binary);
     }
 
     bool dump(const char *p, size_t sz) {
+        if ( sz > bytes_remaining_ ) {
+            bytes_remaining_ = 0;
+            return false;
+        }
+        bytes_remaining_ -= sz;
         ofs_.write(p, sz);
-        return true;
+        return ofs_.good();
     }
 
     template<typename V>
     typename std::enable_if<type_traits_internal::IsTriviallyCopyable<V>::value, bool>::type
     dump(const V& v) {
-        ofs_.write(reinterpret_cast<const char *>(&v), sizeof(V));
-        return true;
+        return dump(reinterpret_cast<const char *>(&v), sizeof(V));
+    }
+
+    bool close() {
+        ofs_.close();
+        return ofs_.good();
     }
 
 private:
     std::ofstream ofs_;
+    size_t bytes_remaining_;
 };
 
 
 class BinaryInputArchive {
 public:
-    BinaryInputArchive(const char * file_path) {
+    BinaryInputArchive(const char * file_path,
+                       size_t bytes_remaining = std::numeric_limits<size_t>::max())
+        : bytes_remaining_(bytes_remaining) {
         ifs_.open(file_path, std::ios_base::binary);
     }
 
     bool load(char* p, size_t sz) {
+        if ( sz > bytes_remaining_ ) {
+            bytes_remaining_ = 0;
+            return false;
+        }
+        bytes_remaining_ -= sz;
         ifs_.read(p, sz);
-        return true;
+        return ifs_.good();
     }
 
     template<typename V>
     typename std::enable_if<type_traits_internal::IsTriviallyCopyable<V>::value, bool>::type
     load(V* v) {
-        ifs_.read(reinterpret_cast<char *>(v), sizeof(V));
-        return true;
+        return load(reinterpret_cast<char *>(v), sizeof(V));
     }
 
 private:
     std::ifstream ifs_;
+    size_t bytes_remaining_;
 };
 
 } // namespace phmap

test/io/serialize.cpp

@@ -1,9 +1,19 @@
 #include <catch.hpp>
 
+#include <filesystem>
+
 #include <mockturtle/io/serialize.hpp>
 
 using namespace mockturtle;
 
+#if __GNUC__ == 7
+namespace fs = std::experimental::filesystem::v1;
+#else
+namespace fs = std::filesystem;
+#endif
+
+static constexpr char file_name[] = "aig.dmp" ;
+
 TEST_CASE( "serialize aig_network into a file", "[serialize]" )
 {
   aig_network aig;
@@ -19,10 +29,10 @@ TEST_CASE( "serialize aig_network into a file", "[serialize]" )
   aig.create_po( f5 );
 
   /* serialize */
-  serialize_network( aig, "aig.dmp" );
+  serialize_network( aig, file_name );
 
   /* deserialize */
-  aig_network aig2 = deserialize_network( "aig.dmp" );
+  aig_network aig2 = deserialize_network( file_name );
 
   CHECK( aig.size() == aig2.size() );
   CHECK( aig.num_cis() == aig2.num_cis() );
@@ -46,3 +56,69 @@ TEST_CASE( "serialize aig_network into a file", "[serialize]" )
   CHECK( aig2._storage->nodes[f5.index].children[0u].index == f4.index );
   CHECK( aig2._storage->nodes[f5.index].children[1u].index == f3.index );
 }
+
+static aig_network create_network()
+{
+  aig_network aig;
+
+  const auto a = aig.create_pi();
+  const auto b = aig.create_pi();
+
+  const auto f1 = aig.create_nand( a, b );
+  const auto f3 = aig.create_nand( b, f1 );
+  const auto f4 = aig.create_nand( a, f1 );
+  const auto f5 = aig.create_nand( f4, f3 );
+  aig.create_po( f5 );
+
+  return aig;
+}
+
+// These numbers were chosen to get 100% coverage of the `return false`
+// error paths in `serialize.hpp`.
+//
+// To find a value that gives coverage of a particular `return false`
+// statement, change the loops below to iterate from 0 to 1000000,
+// configure with `-DCMAKE_BUILD_TYPE=DEBUG` and run in a debugger
+// with a breakpoint set at the line of interest. When the breakpoint
+// is hit, get the value of `size` from its stack frame and add it
+// to this list.
+static constexpr int truncate_sizes[] =
+{
+  0, 8, 16, 32, 40, 344, 352, 368, 376, 384, 672120
+};
+
+TEST_CASE( "write errors are propagated", "[serialize]" )
+{
+  aig_network aig = create_network();
+
+  serialize_network( aig, file_name );
+  size_t file_size = fs::file_size( file_name );
+  INFO("File size " << file_size);
+
+  for ( size_t size : truncate_sizes ) {
+    if ( size >= file_size )
+    {
+      break;
+    }
+    phmap::BinaryOutputArchive output ( file_name, size );
+    CHECK_FALSE( serialize_network_fallible( aig, output ) );
+  }
+}
+
+TEST_CASE( "read errors are propagated", "[serialize]" )
+{
+  aig_network aig = create_network();
+
+  serialize_network( aig, file_name );
+  size_t file_size = fs::file_size( file_name );
+  INFO("File size " << file_size);
+
+  for ( size_t size : truncate_sizes ) {
+    if ( size >= file_size )
+    {
+      break;
+    }
+    phmap::BinaryInputArchive input ( file_name, size );
+    CHECK_FALSE( deserialize_network_fallible( input ).has_value() );
+  }
+}

test/io/write_aiger.cpp

@@ -78,7 +78,6 @@ TEST_CASE( "write AIG for XOR into AIGERfile", "[write_aiger]" )
   seq_buffer<char> buffer;
   std::ostream os( &buffer );
   write_aiger( aig, os );
-  write_aiger( aig, "test.aig" );
 
   CHECK( buffer.data() ==
          std::vector<char>{

test/test.cpp

@@ -1,6 +1,43 @@
-#define CATCH_CONFIG_MAIN
+#define CATCH_CONFIG_RUNNER
+
 #include <catch.hpp>
 
-#include <mockturtle/mockturtle.hpp>
+#include <filesystem>
+#include <random>
+
+#include <fmt/format.h>
+
+#if __GNUC__ == 7
+namespace fs = std::experimental::filesystem::v1;
+#else
+namespace fs = std::filesystem;
+#endif
+
+// Insecure but portable creation of a temporary directory
+static fs::path temp_directory()
+{
+  std::random_device rd;
+  std::mt19937_64 generator( rd() );
+  std::uniform_int_distribution<uint64_t> distribution( 0, std::numeric_limits<uint64_t>::max() );
+  uint64_t random_number = distribution( generator );
+
+  fs::path path = fs::temp_directory_path();
+  path /= fmt::format( "mockturtle_test_{:x}", random_number );
+  return path;
+}
+
+int main(int argc, char* argv[])
+{
+  auto temp_dir = temp_directory();
+  fs::create_directory( temp_dir );
+  fs::current_path( temp_dir );
+
+  int exit_code = Catch::Session().run(argc, argv);
+
+  if ( !exit_code ) {
+    fs::current_path( ".." );
+    fs::remove_all( temp_dir );
+  }
 
-#include <iostream>
+  return exit_code;
+}