reimplement asserts with a safer approach

The original asserts weren't very useful in debug mode as they
were lacking information on where they were being triggered and
were also unreliable and dangerous as they could result in
important code being removed and trigger crashes (even in non
debug mode).

Instead of implementing one generic assert for both modes, build
a more useful one for each one, so PCRE2_UNREACHABLE() could be
also used in non debug builds to help with optimization.

Reinstate all original assertions to use the new versions, which
will have the sideeffect of fixing indentation issues introduced
in the original, and include additional asserts that were provided
as the original ones were being checked.

While at it, update the cmake and CI to help with testing.

.github/workflows/build.yml

@@ -16,10 +16,10 @@ jobs:
         run: ./autogen.sh
 
       - name: Configure
-        run: ./configure CPPFLAGS='-Wall -Wextra' --enable-jit --enable-pcre2-16 --enable-pcre2-32
+        run: ./configure --enable-jit --enable-pcre2-16 --enable-pcre2-32
 
       - name: Build
-        run: make -j2
+        run: make -j2 CPPFLAGS='-Wall -Wextra -Werror'
 
       - name: Test (main test script)
         run: ./RunTest
@@ -50,10 +50,10 @@ jobs:
         run: ./autogen.sh
 
       - name: Configure
-        run: ./configure CPPFLAGS='-Wall -Wextra' --enable-jit --enable-pcre2-16 --enable-pcre2-32
+        run: ./configure --enable-jit --enable-pcre2-16 --enable-pcre2-32
 
       - name: Build
-        run: make -j2
+        run: make -j2 CPPFLAGS='-Wall -Wextra -Werror'
 
       - name: Test (main test script)
         run: ./RunTest
@@ -77,7 +77,7 @@ jobs:
           submodules: true
 
       - name: Configure
-        run: cmake -DPCRE2_SUPPORT_JIT=ON -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=ON -DCMAKE_OSX_ARCHITECTURES='arm64;x86_64' -DCMAKE_C_FLAGS='-Wall -Wextra' -B build
+        run: cmake -DPCRE2_SUPPORT_JIT=ON -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=ON -DCMAKE_OSX_ARCHITECTURES='arm64;x86_64' -DCMAKE_C_FLAGS='-Wall -Wextra' -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -B build
 
       - name: Build
         run: cmake --build build
@@ -112,7 +112,7 @@ jobs:
           submodules: true
 
       - name: Configure
-        run: cmake -DPCRE2_SUPPORT_JIT=ON -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=ON -B build -A Win32
+        run: cmake -DPCRE2_SUPPORT_JIT=ON -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=ON -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -B build -A Win32
 
       - name: Build
         run: cmake --build build

CMakeLists.txt

@@ -145,8 +145,6 @@ INCLUDE(CheckIncludeFile)
 INCLUDE(CheckTypeSize)
 INCLUDE(GNUInstallDirs) # for CMAKE_INSTALL_LIBDIR
 
-CHECK_INCLUDE_FILE(stdio.h      HAVE_STDIO_H)
-CHECK_INCLUDE_FILE(stdlib.h     HAVE_STDLIB_H)
 CHECK_INCLUDE_FILE(assert.h     HAVE_ASSERT_H)
 CHECK_INCLUDE_FILE(dirent.h     HAVE_DIRENT_H)
 CHECK_INCLUDE_FILE(sys/stat.h   HAVE_SYS_STAT_H)
@@ -1164,6 +1162,7 @@ IF(PCRE2_SHOW_REPORT)
   MESSAGE(STATUS "  Build 8 bit PCRE2 library ....... : ${PCRE2_BUILD_PCRE2_8}")
   MESSAGE(STATUS "  Build 16 bit PCRE2 library ...... : ${PCRE2_BUILD_PCRE2_16}")
   MESSAGE(STATUS "  Build 32 bit PCRE2 library ...... : ${PCRE2_BUILD_PCRE2_32}")
+  MESSAGE(STATUS "  Include debugging code ...........: ${PCRE2_DEBUG}")
   MESSAGE(STATUS "  Enable JIT compiling support .... : ${PCRE2_SUPPORT_JIT}")
   MESSAGE(STATUS "  Use SELinux allocator in JIT .... : ${PCRE2_SUPPORT_JIT_SEALLOC}")
   MESSAGE(STATUS "  Enable Unicode support .......... : ${PCRE2_SUPPORT_UNICODE}")

config-cmake.h.in

@@ -6,8 +6,6 @@
 #cmakedefine HAVE_BUILTIN_UNREACHABLE 1
 #cmakedefine HAVE_ATTRIBUTE_UNINITIALIZED 1
 #cmakedefine HAVE_DIRENT_H 1
-#cmakedefine HAVE_STDIO_H 1
-#cmakedefine HAVE_STDLIB_H 1
 #cmakedefine HAVE_SYS_STAT_H 1
 #cmakedefine HAVE_SYS_TYPES_H 1
 #cmakedefine HAVE_UNISTD_H 1

src/pcre2_auto_possess.c

@@ -199,7 +199,7 @@ static BOOL
 check_char_prop(uint32_t c, unsigned int ptype, unsigned int pdata,
   BOOL negated)
 {
-BOOL ok;
+BOOL ok, rc;
 const uint32_t *p;
 const ucd_record *prop = GET_UCD(c);
 
@@ -240,13 +240,13 @@ switch(ptype)
     {
     HSPACE_CASES:
     VSPACE_CASES:
-    return negated;
+    rc = negated;
+    break;
 
     default:
-    return (PRIV(ucp_gentype)[prop->chartype] == ucp_Z) == negated;
+    rc = (PRIV(ucp_gentype)[prop->chartype] == ucp_Z) == negated;
     }
-  PCRE2_UNREACHABLE(); /* Control never reaches here */
-  break;
+  return rc;
 
   case PT_WORD:
   return (PRIV(ucp_gentype)[prop->chartype] == ucp_L ||
@@ -260,7 +260,7 @@ switch(ptype)
     if (c < *p) return !negated;
     if (c == *p++) return negated;
     }
-  PCRE2_UNREACHABLE(); /* Control never reaches here */
+  PCRE2_DEBUG_UNREACHABLE(); /* Control should never reach here */
   break;
 
   /* Haven't yet thought these through. */
@@ -456,6 +456,7 @@ switch(c)
        {
        /* Early return if there is not enough space. This should never
        happen, since all clists are shorter than 5 character now. */
+       PCRE2_DEBUG_UNREACHABLE();
        list[2] = code[0];
        list[3] = code[1];
        return code;
@@ -1121,7 +1122,7 @@ for(;;)
   if (list[1] == 0) return TRUE;
   }
 
-  PCRE2_UNREACHABLE(); /* Control never reaches here */
+PCRE2_DEBUG_UNREACHABLE(); /* Control should never reach here */
 }
 
 

src/pcre2_compile.c

@@ -1848,8 +1848,8 @@ else
       /* As we know we are at a digit, the only possible error from
       read_number() is a number that is too large to be a group number. Because
       that number might be still valid if read as an octal, errorcodeptr is not
-      set on failure and therefore a bogus value of INT_MAX is set instead that
-      will be used later to properly set the error, if not falling through. */
+      set on failure and therefore a sentinel value of INT_MAX is set instead so
+      it can be used later to properly set the error, if not falling through. */
 
       if (!read_number(&ptr, ptrend, -1, MAX_GROUP_NUMBER, 0, &s, errorcodeptr))
         s = INT_MAX;
@@ -1860,10 +1860,14 @@ else
       if (s < 10 || c >= CHAR_8 || (unsigned)s <= bracount)
         {
         /* s > MAX_GROUP_NUMBER should not be possible because of read_number(),
-        but we keep it just to be safe and because it will also catch the bogus
-        value set on failure of that function. */
+        but we keep it just to be safe and because it will also catch the
+        sentinel value that was set on failure for that function. */
 
-        if ((unsigned)s > MAX_GROUP_NUMBER) *errorcodeptr = ERR61;
+        if ((unsigned)s > MAX_GROUP_NUMBER)
+          {
+          PCRE2_ASSERT(s == INT_MAX);
+          *errorcodeptr = ERR61;
+          }
         else escape = -s;     /* Indicates a back reference */
         break;
         }
@@ -5245,7 +5249,7 @@ for (;;)
     }
   }
 
-  PCRE2_UNREACHABLE(); /* Control never reaches here */
+PCRE2_DEBUG_UNREACHABLE(); /* Control should never reach here */
 }
 
 
@@ -5525,6 +5529,7 @@ have duplicate names. Give an internal error. */
 
 if (i >= cb->names_found)
   {
+  PCRE2_DEBUG_UNREACHABLE();
   *errorcodeptr = ERR53;
   cb->erroroffset = name - cb->start_pattern;
   return FALSE;
@@ -8452,7 +8457,7 @@ for (;; pptr++)
     }         /* End of big switch */
   }           /* End of big loop */
 
-  PCRE2_UNREACHABLE(); /* Control never reaches here */
+PCRE2_DEBUG_UNREACHABLE(); /* Control should never reach here */
 }
 
 
@@ -8758,7 +8763,7 @@ for (;;)
   pptr++;
   }
 
-  PCRE2_UNREACHABLE(); /* Control never reaches here */
+PCRE2_DEBUG_UNREACHABLE(); /* Control should never reach here */
 }
 
 
@@ -9382,6 +9387,7 @@ for (;; pptr++)
     /* This should never occur. */
 
     case META_END:
+    PCRE2_DEBUG_UNREACHABLE();
     return NULL;
 
     /* The data for these items is variable in length. */
@@ -9446,7 +9452,7 @@ for (;; pptr++)
   pptr += meta_extra_lengths[meta];
   }
 
-  PCRE2_UNREACHABLE(); /* Control never reaches here */
+PCRE2_UNREACHABLE(); /* Control never reaches here */
 }
 
 
@@ -10528,7 +10534,8 @@ if ((options & PCRE2_LITERAL) == 0)
           break;
 
           default:
-          PCRE2_UNREACHABLE();
+          /* Fail assertion if new type was added and someone forgot to update this switch */
+          PCRE2_DEBUG_UNREACHABLE();
           }
         break;   /* Out of the table scan loop */
         }

src/pcre2_convert.c

@@ -1137,6 +1137,7 @@ for (int i = 0; i < 2; i++)
     break;
 
     default:
+    PCRE2_DEBUG_UNREACHABLE();
     *bufflenptr = 0;  /* Error offset */
     return PCRE2_ERROR_INTERNAL;
     }
@@ -1160,8 +1161,9 @@ for (int i = 0; i < 2; i++)
 
 /* Normally, we should exit this function in the previous loop, but we
 can't return an API call without a meaningful value, so if something
-went terribly wrong, we then will just report it as an intenal error */
+went terribly wrong, we then will just report it as an internal error */
 
+PCRE2_DEBUG_UNREACHABLE(); /* Control should never reach here */
 return PCRE2_ERROR_INTERNAL;
 }
 

src/pcre2_dfa_match.c

@@ -3570,7 +3570,9 @@ switch(re->newline_convention)
   mb->nltype = NLTYPE_ANYCRLF;
   break;
 
-  default: return PCRE2_ERROR_INTERNAL;
+  default:
+  PCRE2_DEBUG_UNREACHABLE();
+  return PCRE2_ERROR_INTERNAL;
   }
 
 /* Check a UTF string for validity if required. For 8-bit and 16-bit strings,