Make assert handling less error-prone

Prevent my future self or other contributors from using assert in tests and then having that assert not do anything because of the NDEBUG macro.

Make assert handling less error-prone
Prevent my future self or other contributors from using assert in tests and then having that assert not do anything because of the NDEBUG macro.
Jay Berkenbilt
1 parent 92b69246
Showing 28 changed files with 191 additions and 130 deletions
CMakeLists.txt
README-maintainer
check_assert
include/qpdf/QIntC.hh
libqpdf/QPDFWriter.cc
libqpdf/QPDF_encryption.cc
libqpdf/QPDF_optimization.cc
libqpdf/qpdf/assert_debug.h
libqpdf/qpdf/assert_test.h
libtests/arg_parser.cc
libtests/base64.cc
libtests/buffer.cc
libtests/concatenate.cc
libtests/cxx11.cc
libtests/json.cc
libtests/json_handler.cc
libtests/matrix.cc
libtests/pdf_version.cc
libtests/predictors.cc
libtests/qintc.cc
@@ -305,6 +305,10 @@ if(GENERATE_AUTO_JOB)
   add_custom_target(auto_job_files ALL DEPENDS ${auto_job_outputs})
 endif()
+add_test(
+  NAME check-assert
+  COMMAND perl ${qpdf_SOURCE_DIR}/check_assert)
+
 # add_subdirectory order affects test order
 add_subdirectory(include)
 add_subdirectory(libqpdf)
@@ -123,11 +123,23 @@ CODING RULES
   "Code Formatting" section in manual/contributing.rst for details.
   See also "CODE FORMATTING" below.
-* Do not use assert in non-test code for any purpose other than as a
-  sanity check during development that would be safe to remove in
-  production. assert is for strong invariant checking. When developing
-  and using assert for that purpose, make sure to use the Debug
-  configuration since assert is disabled in other configurations.
+* Use of assert:
+
+  * Test code: #include <qpdf/assert_test.h> first.
+  * Debug code: #include <qpdf/assert_debug.h> first and use
+    qpdf_assert_debug instead of assert.
+
+  These rules are enforced by the check-assert test. This practices
+  serves to
+
+  * remind us that assert in release code disappears and so should only
+    be used for debugging; when doing so use a Debug build
+    configuration
+
+  * protect us from using assert in test code without explicitly
+    removing the NDEBUG definition, since that would cause the assert
+    not to actually be testing anything in non-Debug build
+    configurations.
 * In a source file, include the header file that declares the source
   class first followed by a blank line. If a config file is needed
+#!/usr/bin/env perl
+require 5.008;
+use warnings;
+use strict;
+use File::Basename;
+
+my $whoami = basename($0);
+chdir(dirname($0)) or die;
+
+my $errors = 0;
+foreach my $file (glob('*/*.c'), glob('*/*.cc'),
+                  glob('*/*/*.h'), glob('*/*/*.hh'))
+{
+    my $assert_test = 0;
+    if ($file =~ m,^libqpdf/qpdf/assert_,)
+    {
+        next;
+    }
+    open(F, "<$file") or die;
+    my $first_include = undef;
+    while (<F>)
+    {
+        if (m,^\s*#\s*include <qpdf/assert_(.*?).h>,)
+        {
+            if ($1 eq 'test')
+            {
+                $assert_test = 1;
+            }
+            if (defined $first_include)
+            {
+                error("$file:$.: qpdf/assert header must be first");
+            }
+        }
+        if (m,^\s*#\s*include <(.*?)>,)
+        {
+            my $header = $1;
+            if (($header eq 'cassert') || ($header eq 'assert.h'))
+            {
+                error("$file:$.: assert.h and cassert are not allowed --" .
+                      " use one of the qpdf/assert_ files instead");
+            }
+            $first_include = 1;
+        }
+        if ((! $assert_test) && (m/assert\(/))
+        {
+            error("$file:$.: call qpdf_assert_debug instead of assert");
+        }
+    }
+    close(F);
+}
+if ($errors)
+{
+    die "$whoami: errors detected\n";
+}
+print "$whoami: no incorrect use of assert found\n";
+
+sub error
+{
+    my $msg = shift;
+    warn $msg, "\n";
+    $errors = 1;
+}
@@ -24,7 +24,6 @@
 #include <qpdf/DLL.h>
 #include <qpdf/Types.h>
-#include <cassert>
 #include <iostream>
 #include <limits>
 #include <locale>
-#include <qpdf/qpdf-config.h> // include first for large file support
+#include <qpdf/assert_debug.h>
+
+#include <qpdf/qpdf-config.h> // include early for large file support
 #include <qpdf/QPDFWriter.hh>
@@ -21,7 +23,6 @@
 #include <qpdf/RC4.hh>
 #include <algorithm>
-#include <cassert>
 #include <stdlib.h>
 QPDFWriter::Members::Members(QPDF& pdf) :
@@ -996,7 +997,7 @@ QPDFWriter::writePad(int nspaces)
 Pipeline*
 QPDFWriter::pushPipeline(Pipeline* p)
 {
-    assert(dynamic_cast<Pl_Count*>(p) == 0);
+    qpdf_assert_debug(dynamic_cast<Pl_Count*>(p) == 0);
     this->m->pipeline_stack.push_back(p);
     return p;
 }
@@ -1027,9 +1028,9 @@ QPDFWriter::PipelinePopper::~PipelinePopper()
     if (stack_id.empty()) {
         return;
     }
-    assert(qw->m->pipeline_stack.size() >= 2);
+    qpdf_assert_debug(qw->m->pipeline_stack.size() >= 2);
     qw->m->pipeline->finish();
-    assert(
+    qpdf_assert_debug(
         dynamic_cast<Pl_Count*>(qw->m->pipeline_stack.back()) ==
         qw->m->pipeline);
     // It might be possible for this assertion to fail if
@@ -1038,7 +1039,7 @@ QPDFWriter::PipelinePopper::~PipelinePopper()
     // which two dynamically allocated PipelinePopper objects ever
     // exist at the same time, so the assertion will fail if they get
     // popped out of order from automatic destruction.
-    assert(qw->m->pipeline->getIdentifier() == stack_id);
+    qpdf_assert_debug(qw->m->pipeline->getIdentifier() == stack_id);
     delete qw->m->pipeline_stack.back();
     qw->m->pipeline_stack.pop_back();
     while (dynamic_cast<Pl_Count*>(qw->m->pipeline_stack.back()) == 0) {
@@ -1109,9 +1110,9 @@ QPDFWriter::pushMD5Pipeline(PipelinePopper&amp; pp)
         throw std::logic_error("Deterministic ID computation enabled after ID"
                                " generation has already occurred.");
     }
-    assert(this->m->deterministic_id);
-    assert(this->m->md5_pipeline == 0);
-    assert(this->m->pipeline->getCount() == 0);
+    qpdf_assert_debug(this->m->deterministic_id);
+    qpdf_assert_debug(this->m->md5_pipeline == 0);
+    qpdf_assert_debug(this->m->pipeline->getCount() == 0);
     this->m->md5_pipeline = new Pl_MD5("qpdf md5", this->m->pipeline);
     this->m->md5_pipeline->persistAcrossFinish(true);
     // Special case code in popPipelineStack clears this->m->md5_pipeline
@@ -1123,8 +1124,8 @@ QPDFWriter::pushMD5Pipeline(PipelinePopper&amp; pp)
 void
 QPDFWriter::computeDeterministicIDData()
 {
-    assert(this->m->md5_pipeline != 0);
-    assert(this->m->deterministic_id_data.empty());
+    qpdf_assert_debug(this->m->md5_pipeline != 0);
+    qpdf_assert_debug(this->m->deterministic_id_data.empty());
     this->m->deterministic_id_data = this->m->md5_pipeline->getHexDigest();
     this->m->md5_pipeline->enable(false);
 }
@@ -1786,7 +1787,7 @@ QPDFWriter::writeObjectStream(QPDFObjectHandle object)
     // object stream that we are generating from scratch.
     QPDFObjGen old_og = object.getObjGen();
-    assert(old_og.getGen() == 0);
+    qpdf_assert_debug(old_og.getGen() == 0);
     int old_id = old_og.getObj();
     int new_id = this->m->obj_renumber[old_og];
@@ -3003,7 +3004,7 @@ QPDFWriter::writeLinearized()
         closeObject(lindict_id);
         static int const pad = 200;
         int spaces = QIntC::to_int(pos - this->m->pipeline->getCount() + pad);
-        assert(spaces >= 0);
+        qpdf_assert_debug(spaces >= 0);
         writePad(spaces);
         writeString("\n");
@@ -3177,7 +3178,7 @@ QPDFWriter::writeLinearized()
                     need_xref_stream ? 0 : 1);
                 computeDeterministicIDData();
                 pp_md5 = 0;
-                assert(this->m->md5_pipeline == 0);
+                qpdf_assert_debug(this->m->md5_pipeline == 0);
             }
             // Close first pass pipeline
@@ -3377,6 +3378,6 @@ QPDFWriter::writeStandard()
             "QPDFWriter standard deterministic ID",
             this->m->object_stream_to_objects.empty() ? 0 : 1);
         pp_md5 = 0;
-        assert(this->m->md5_pipeline == 0);
+        qpdf_assert_debug(this->m->md5_pipeline == 0);
     }
 }
 // This file implements methods from the QPDF class that involve
 // encryption.
+#include <qpdf/assert_debug.h>
+
 #include <qpdf/QPDF.hh>
 #include <qpdf/QPDFExc.hh>
@@ -15,7 +17,6 @@
 #include <qpdf/RC4.hh>
 #include <algorithm>
-#include <cassert>
 #include <string.h>
 static unsigned char const padding_string[] = {
@@ -288,7 +289,7 @@ hash_V5(
             ++round_number;
             std::string K1 = password + K + udata;
-            assert(K.length() >= 32);
+            qpdf_assert_debug(K.length() >= 32);
             std::string E = process_with_aes(
                 K.substr(0, 16),
                 true,
 // See the "Optimization" section of the manual.
+#include <qpdf/assert_debug.h>
+
 #include <qpdf/QPDF.hh>
 #include <qpdf/QPDFExc.hh>
 #include <qpdf/QPDF_Array.hh>
 #include <qpdf/QPDF_Dictionary.hh>
 #include <qpdf/QTC.hh>
-#include <cassert>
 QPDF::ObjUser::ObjUser() :
     ou_type(ou_bad),
@@ -18,14 +19,14 @@ QPDF::ObjUser::ObjUser(user_e type) :
     ou_type(type),
     pageno(0)
 {
-    assert(type == ou_root);
+    qpdf_assert_debug(type == ou_root);
 }
 QPDF::ObjUser::ObjUser(user_e type, int pageno) :
     ou_type(type),
     pageno(pageno)
 {
-    assert((type == ou_page) || (type == ou_thumb));
+    qpdf_assert_debug((type == ou_page) || (type == ou_thumb));
 }
 QPDF::ObjUser::ObjUser(user_e type, std::string const& key) :
@@ -33,7 +34,7 @@ QPDF::ObjUser::ObjUser(user_e type, std::string const&amp; key) :
     pageno(0),
     key(key)
 {
-    assert((type == ou_trailer_key) || (type == ou_root_key));
+    qpdf_assert_debug((type == ou_trailer_key) || (type == ou_root_key));
 }
 bool
+/*
+ * Include this file to use assert in regular code for
+ * debugging/strong sanity checking, knowing that the assert will be
+ * disabled in release code. Use qpdf_assert_debug in the code.
+ */
+
+/* assert_debug and assert_test intentionally use the same
+ * guard. Search for assert in README-MAINTAINER.
+ */
+#ifdef QPDF_ASSERT_H
+# error "At most one qpdf/assert header may be included at most one time"
+#else
+# define QPDF_ASSERT_H
+
+# include <assert.h>
+# define qpdf_assert_debug assert
+
+#endif /* QPDF_ASSERT_H */
+/*
+ * Include this file to use assert in regular code for
+ * debugging/strong sanity checking, knowing that the assert will be
+ * disabled in release code. Use qpdf_debug_assert in the code.
+ */
+
+/* assert_debug and assert_test intentionally use the same
+ * guard. Search for assert in README-MAINTAINER.
+ */
+#ifdef QPDF_ASSERT_H
+# error "At most one qpdf/assert header may be included at most one time"
+#else
+# define QPDF_ASSERT_H
+
+# ifdef NDEBUG
+#  undef NDEBUG
+# endif
+# include <assert.h>
+
+#endif /* QPDF_ASSERT_H */
+#include <qpdf/assert_test.h>
+
 #include <qpdf/QPDFArgParser.hh>
 #include <qpdf/QPDFUsage.hh>
 #include <qpdf/QUtil.hh>
 #include <cstring>
 #include <iostream>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 class ArgParser
 {
   public:
+#include <qpdf/assert_test.h>
+
 #include <qpdf/Pl_Base64.hh>
 #include <qpdf/Pl_StdioFile.hh>
 #include <qpdf/QUtil.hh>
-#include <cassert>
 #include <cstdlib>
 #include <cstring>
 #include <iostream>
+#include <qpdf/assert_test.h>
+
 #include <qpdf/Pl_Buffer.hh>
 #include <qpdf/Pl_Count.hh>
 #include <qpdf/Pl_Discard.hh>
@@ -7,12 +9,6 @@
 #include <stdexcept>
 #include <stdlib.h>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 static unsigned char*
 uc(char const* s)
 {
+#include <qpdf/assert_test.h>
+
 #include <qpdf/Pl_Buffer.hh>
 #include <qpdf/Pl_Concatenate.hh>
 #include <qpdf/Pl_Flate.hh>
 #include <qpdf/QUtil.hh>
 #include <iostream>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 static void
 pipeStringAndFinish(Pipeline* p, std::string const& str)
 {
+#include <qpdf/assert_test.h>
+
 #include <cstdint>
 #include <cstdlib>
 #include <cstring>
@@ -9,12 +11,6 @@
 #include <type_traits>
 #include <vector>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 // Functional programming
 // Function that returns a callable in the form of a lambda
+#include <qpdf/assert_test.h>
+
 #include <qpdf/JSON.hh>
 #include <qpdf/QPDFObjectHandle.hh>
 #include <iostream>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 static void
 check(JSON const& j, std::string const& exp)
 {
+#include <qpdf/assert_test.h>
+
 #include <qpdf/JSONHandler.hh>
 #include <qpdf/QPDFUsage.hh>
 #include <qpdf/QUtil.hh>
 #include <iostream>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 static void
 print_null(std::string const& path)
 {
+#include <qpdf/assert_test.h>
+
 #include <qpdf/QPDFMatrix.hh>
 #include <qpdf/QUtil.hh>
 #include <iostream>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 static void
 check(QPDFMatrix const& m, std::string const& exp)
 {
+#include <qpdf/assert_test.h>
+
 #include <qpdf/PDFVersion.hh>
 #include <iostream>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 int
 main()
 {
+#include <qpdf/assert_test.h>
+
 #include <qpdf/Pl_PNGFilter.hh>
 #include <qpdf/Pl_StdioFile.hh>
 #include <qpdf/Pl_TIFFPredictor.hh>
@@ -9,12 +11,6 @@
 #include <stdlib.h>
 #include <string.h>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 void
 run(char const* filename,
     char const* filter,
+#include <qpdf/assert_test.h>
+
 #include <qpdf/QIntC.hh>
 #include <stdint.h>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 #define try_convert(exp_pass, fn, i) \
  try_convert_real(#fn "(" #i ")", exp_pass, fn, i)
+#include <qpdf/assert_test.h>
+
 #include <qpdf/Pl_Buffer.hh>
 #include <qpdf/QPDFSystemError.hh>
 #include <qpdf/QUtil.hh>
@@ -17,12 +19,6 @@
 # include <unistd.h>
 #endif
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 template <class int_T>
 void
 test_to_number(
+#include <qpdf/assert_test.h>
+
 #include <qpdf/Pl_RC4.hh>
 #include <qpdf/Pl_StdioFile.hh>
 #include <qpdf/QIntC.hh>
@@ -8,12 +10,6 @@
 #include <stdlib.h>
 #include <string.h>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 static void
 other_tests()
 {
+#include <qpdf/assert_test.h>
+
 #include <qpdf/SparseOHArray.hh>
 #include <iostream>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 int
 main()
 {
@@ -29,6 +29,16 @@ endforeach()
 target_include_directories(qpdf-ctest PRIVATE ${CMAKE_BINARY_DIR}/libqpdf)
 target_include_directories(sizes PRIVATE ${JPEG_INCLUDE})
+set(needs_private_headers
+  test_large_file
+  test_driver
+  qpdf-ctest
+  qpdfjob-ctest
+  )
+foreach(TARGET ${needs_private_headers})
+  target_include_directories(${TARGET} PRIVATE ${CMAKE_SOURCE_DIR}/libqpdf)
+endforeach()
+
 foreach(B qpdf test_unicode_filenames fix-qdf test_shell_glob)
   if(WINDOWS_WMAIN_COMPILE)
     target_compile_options(${B} PRIVATE ${WINDOWS_WMAIN_COMPILE})
+#include <qpdf/assert_test.h>
+
 #include <qpdf/qpdf-c.h>
 #include <errno.h>
 #include <stdio.h>
@@ -6,12 +8,6 @@
 #include <qpdf/qpdf-config.h> // for LL_FMT -- special case in build
-#ifdef NDEBUG
-/* We need assert even in a release build for test code. */
-# undef NDEBUG
-#endif
-#include <assert.h>
-
 static char* whoami = 0;
 static qpdf_data qpdf = 0;
+#include <qpdf/assert_test.h>
+
 #include <qpdf/qpdfjob-c.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#ifdef NDEBUG
-/* We need assert even in a release build for test code. */
-# undef NDEBUG
-#endif
-#include <assert.h>
-
 #ifndef QPDF_NO_WCHAR_T
 static void
 wide_test()
+#include <qpdf/assert_test.h>
+
 // This program tests miscellaneous functionality in the qpdf library
 // that we don't want to pollute the qpdf program with.
@@ -32,12 +34,6 @@
 #include <stdlib.h>
 #include <string.h>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 static char const* whoami = 0;
 void
+#include <qpdf/assert_test.h>
+
 // NOTE: This test program doesn't do anything special to enable large
 // file support.  This is important since it verifies that programs
 // don't have to do anything special -- all the work is done
@@ -14,12 +16,6 @@
 #include <stdlib.h>
 #include <string.h>
-#ifdef NDEBUG
-// We need assert even in a release build for test code.
-# undef NDEBUG
-#endif
-#include <cassert>
-
 // Run "test_large_file write small a.pdf" to get a PDF file that you
 // can look at in a reader.