Fix phpGH-8065: Rework checksum skip list in a pragmatic way

Fixes phpGH-8065 (see also for analysis)

The class inheritance cache must be excluded from the checksum
computation because it may change (legally) in between the computation
and recomputation.
Furthermore the handler pointer in the zend_op structure may be modified
by the JIT, and the op_array->reserved[] could be changed as well.

This approach is based on iluuu1994's original approach, with some
slight changes. First let's explain the main idea. The idea is to
keep a "skip list" of offsets to skip in the checksum computation.
This will include the offsets described above
(class inheritance cache, handler, reserved). These skips are of size
pointer_size (equals to the native pointer size).

It differs in the sense that this is a pragmatic approach. It was found
that in the original approach it was very difficult to figure out what
can be exactly changed when, and let to redundant computation of the CFG
for example. Furthermore, such a process is also error-prone and complex
to maintain. Instead of that, we just always skip the handler pointer
and the reserved area. The positive is that it leads to code which is
easier to understand and maintain. The negative is that we won't catch
corruptions in those memory areas. However, I believe this is acceptable
because of the following reasons:
* We'd have to be very unlucky to *only* have corruptions in *those*
  areas. Usually, corruptions are not that well-targetted.
* A checksum isn't a fully error-proof thing either, it's very possible
  that there is a corruption in other areas that the checksum doesn't
  capture.

Let's now talk about the implementation. Keeping all those entries in a
skip list wouldn't be efficient: we'd need an entry for every opcode,
which would lead to many many entries. However, this approach actually
implements a "compression" scheme. Instead of just storing the offset,
we actually store a triple:
<offset, size of the area to be checked, amount of repetitions>.
The offset indicates which pointer needs to be skipped. If the number of
repetitions is greater than zero, it will do the following
`repetitions` times:
* Checksum the bytes at
  [offset + pointer_size, offset + pointer_size + size of area to be checked]
* Move the offset to after the area that was just checksummed +
  pointer_size.
This allows us to only use one skip list entry for all the opcodes in an
op_array.

The offsets also aren't checked in the zend_adler32() function itself,
the zend_accel_script_checksum() function will checksum in "blocks"
of bytes that should not be skipped, by setting the size for
zend_alder32() in such a way that it computes the checksum until the
next offset to skip. The main advantage of this is better performance,
since there are fewer checks, and the accelerated SSE2 (or future other
accelerated) routines can keep being used.

Finally some other minor differences:
* We precompute the exact size needed for the skip list. This avoids
  reallocations and also fixes one issue where there was a warning about
  the computed size not being equals to the actual size.

Author: nielsdos

ext/opcache/ZendAccelerator.c

@@ -4763,9 +4763,8 @@ static int accel_finish_startup(void)
 			ZCG(cwd_key_len) = 0;
 			ZCG(cwd_check) = 1;
 
-			ZCG(checksum_skip_list) = NULL;
-			ZCG(checksum_skip_list_count) = 0;
-			ZCG(checksum_skip_list_capacity) = 0;
+			ZCG(mem_checksum_skip_list) = NULL;
+			ZCG(mem_checksum_skip_list_count) = 0;
 
 			if (accel_preload(ZCG(accel_directives).preload, in_child) != SUCCESS) {
 				ret = FAILURE;

ext/opcache/ZendAccelerator.h

@@ -109,6 +109,19 @@ typedef enum _zend_accel_restart_reason {
 	ACCEL_RESTART_USER    /* restart scheduled by opcache_reset() */
 } zend_accel_restart_reason;
 
+typedef struct _zend_accel_skip_list_entry {
+	/* The offset indicates which byte offset within the memory block must be skipped.
+	 * The size of the skip is equal to the system's pointer size. */
+	uint32_t offset;
+	/* To prevent creating a huge list with a lot of entries, we use a compression scheme
+	 * based on the following two fields. If repetitions > 0, then the checksum algorithm
+	 * will repeat `repetitions` times checksumming `checked_area_size` bytes, followed
+	 * by skipping a pointer. For example, we use this to skip the `zend_op->handler` pointer.
+	 * This allows us to only create one skip list entry per op array to handle *all* the opcodes. */
+	uint32_t checked_area_size;
+	uint32_t repetitions;
+} zend_accel_skip_list_entry;
+
 typedef struct _zend_persistent_script {
 	zend_script    script;
 	zend_long      compiler_halt_offset;   /* position of __HALT_COMPILER or -1 */
@@ -122,8 +135,10 @@ typedef struct _zend_persistent_script {
 
 	void          *mem;                    /* shared memory area used by script structures */
 	size_t         size;                   /* size of used shared memory */
-	/* list of offsets relative to (mem + ZEND_ALIGNED_SIZE(sizeof(zend_persistent_script))) to be skipped in the checksum calculation */
-	uint32_t      *checksum_skip_list;
+
+	/* Some bytes must be skipped in the checksum calculation because they could've changed (legally)
+	 * since the last checksum calculation. */
+	zend_accel_skip_list_entry *mem_checksum_skip_list;
 
 	/* All entries that shouldn't be counted in the ADLER32
 	 * checksum must be declared in this struct
@@ -228,9 +243,8 @@ typedef struct _zend_accel_globals {
 	zend_string             key;
 	char                    _key[MAXPATHLEN * 8];
 
-	uint32_t               *checksum_skip_list;
-	uint32_t                checksum_skip_list_count;
-	uint32_t                checksum_skip_list_capacity;
+	zend_accel_skip_list_entry *mem_checksum_skip_list;
+	uint32_t mem_checksum_skip_list_count;
 } zend_accel_globals;
 
 typedef struct _zend_string_table {

ext/opcache/jit/zend_jit.c

@@ -36,6 +36,7 @@
 #include "Optimizer/zend_func_info.h"
 #include "Optimizer/zend_ssa.h"
 #include "Optimizer/zend_inference.h"
+#include "Optimizer/zend_call_graph.h"
 #include "Optimizer/zend_dump.h"
 
 #if ZEND_JIT_TARGET_X86
@@ -1266,7 +1267,7 @@ static int zend_may_overflow(const zend_op *opline, const zend_ssa_op *ssa_op, c
 	}
 }
 
-ZEND_EXT_API int zend_jit_build_cfg(const zend_op_array *op_array, zend_cfg *cfg)
+static int zend_jit_build_cfg(const zend_op_array *op_array, zend_cfg *cfg)
 {
 	uint32_t flags;
 

ext/opcache/jit/zend_jit.h

@@ -19,8 +19,6 @@
 #ifndef HAVE_JIT_H
 #define HAVE_JIT_H
 
-#include "Optimizer/zend_call_graph.h"
-
 #if defined(__x86_64__) || defined(i386) || defined(ZEND_WIN32)
 # define ZEND_JIT_TARGET_X86   1
 # define ZEND_JIT_TARGET_ARM64 0
@@ -156,7 +154,6 @@ ZEND_EXT_API void zend_jit_activate(void);
 ZEND_EXT_API void zend_jit_deactivate(void);
 ZEND_EXT_API void zend_jit_status(zval *ret);
 ZEND_EXT_API void zend_jit_restart(void);
-ZEND_EXT_API int zend_jit_build_cfg(const zend_op_array *op_array, zend_cfg *cfg);
 
 typedef struct _zend_lifetime_interval zend_lifetime_interval;
 typedef struct _zend_life_range zend_life_range;

ext/opcache/jit/zend_jit_trace.c

@@ -16,8 +16,6 @@
    +----------------------------------------------------------------------+
 */
 
-#include "zend_persist.h"
-
 static zend_op_array dummy_op_array;
 static zend_jit_trace_info *zend_jit_traces = NULL;
 static const void **zend_jit_exit_groups = NULL;
@@ -8264,7 +8262,6 @@ static int zend_jit_setup_hot_trace_counters(zend_op_array *op_array)
 				if (cfg.blocks[i].flags & ZEND_BB_LOOP_HEADER) {
 					/* loop header */
 					opline = op_array->opcodes + cfg.blocks[i].start;
-					checksum_skip_list_add(&opline->handler);
 					if (!(ZEND_OP_TRACE_INFO(opline, jit_extension->offset)->trace_flags & ZEND_JIT_TRACE_UNSUPPORTED)) {
 						opline->handler = (const void*)zend_jit_loop_trace_counter_handler;
 						if (!ZEND_OP_TRACE_INFO(opline, jit_extension->offset)->counter) {
@@ -8289,7 +8286,6 @@ static int zend_jit_setup_hot_trace_counters(zend_op_array *op_array)
 			}
 		}
 
-		checksum_skip_list_add(&opline->handler);
 		if (!ZEND_OP_TRACE_INFO(opline, jit_extension->offset)->trace_flags) {
 			/* function entry */
 			opline->handler = (const void*)zend_jit_func_trace_counter_handler;

ext/opcache/zend_accelerator_util_funcs.c

@@ -299,33 +299,18 @@ zend_op_array* zend_accel_load_script(zend_persistent_script *persistent_script,
 #define ADLER32_DO4(buf, i)     ADLER32_DO2(buf, i); ADLER32_DO2(buf, i + 2);
 #define ADLER32_DO8(buf, i)     ADLER32_DO4(buf, i); ADLER32_DO4(buf, i + 4);
 #define ADLER32_DO16(buf)       ADLER32_DO8(buf, 0); ADLER32_DO8(buf, 8);
-#define ADLER32_CHECK_SKIPPED(by) \
-	do {															\
-		offset = buf - start;										\
-		while (*skip_list != 0 && *skip_list < offset) {			\
-			skip_list++;											\
-		}															\
-		skipped = *skip_list != 0 && *skip_list < (offset + (by));	\
-	} while (0)
-
-unsigned int zend_adler32(unsigned int checksum, unsigned char *buf, uint32_t len, uint32_t *skip_list)
+
+unsigned int zend_adler32(unsigned int checksum, unsigned char *buf, uint32_t len)
 {
-	unsigned char *start = buf;
 	unsigned int s1 = checksum & 0xffff;
 	unsigned int s2 = (checksum >> 16) & 0xffff;
 	unsigned char *end;
-	bool skipped;
-	uint32_t offset;
 
 	while (len >= ADLER32_NMAX) {
 		len -= ADLER32_NMAX;
 		end = buf + ADLER32_NMAX;
-		skipped = false;
 		do {
-			ADLER32_CHECK_SKIPPED(16);
-			if (!skipped) {
-				ADLER32_DO16(buf);
-			}
+			ADLER32_DO16(buf);
 			buf += 16;
 		} while (buf != end);
 		s1 %= ADLER32_BASE;
@@ -336,27 +321,16 @@ unsigned int zend_adler32(unsigned int checksum, unsigned char *buf, uint32_t le
 		if (len >= 16) {
 			end = buf + (len & 0xfff0);
 			len &= 0xf;
-			skipped = false;
 			do {
-				ADLER32_CHECK_SKIPPED(16);
-				if (!skipped) {
-					ADLER32_DO16(buf);
-				}
+				ADLER32_DO16(buf);
 				buf += 16;
 			} while (buf != end);
 		}
 		if (len) {
 			end = buf + len;
-			skipped = false;
 			do {
-				ADLER32_CHECK_SKIPPED(1);
-				if (!skipped) {
-					ADLER32_DO1(buf);
-					buf++;
-				} else {
-					// skip_list contains a list of pointers so we skip the entire pointer size
-					buf += sizeof(void*);
-				}
+				ADLER32_DO1(buf);
+				buf++;
 			} while (buf != end);
 		}
 		s1 %= ADLER32_BASE;
@@ -372,20 +346,43 @@ unsigned int zend_accel_script_checksum(zend_persistent_script *persistent_scrip
 	size_t size = persistent_script->size;
 	size_t persistent_script_check_block_size = ((char *)&(persistent_script->dynamic_members)) - (char *)persistent_script;
 	unsigned int checksum = ADLER32_INIT;
-	uint32_t zero = 0;
 
 	if (mem < (unsigned char*)persistent_script) {
-		checksum = zend_adler32(checksum, mem, (unsigned char*)persistent_script - mem, &zero);
+		checksum = zend_adler32(checksum, mem, (unsigned char*)persistent_script - mem);
 		size -= (unsigned char*)persistent_script - mem;
 		mem  += (unsigned char*)persistent_script - mem;
 	}
 
-	checksum = zend_adler32(checksum, mem, persistent_script_check_block_size, &zero);
-	mem  += ZEND_ALIGNED_SIZE(sizeof(*persistent_script));
-	size -= ZEND_ALIGNED_SIZE(sizeof(*persistent_script));
+	checksum = zend_adler32(checksum, mem, persistent_script_check_block_size);
+	mem  += sizeof(*persistent_script);
+	size -= sizeof(*persistent_script);
 
-	if (size > 0) {
-		checksum = zend_adler32(checksum, mem, size, persistent_script->checksum_skip_list);
+	if (UNEXPECTED(size == 0)) {
+		return checksum;
 	}
+
+	const zend_accel_skip_list_entry *skip_list_entry = persistent_script->mem_checksum_skip_list;
+
+	size_t offset = 0;
+	uint32_t next_stop_read;
+	while ((next_stop_read = skip_list_entry->offset) != 0) {
+		checksum = zend_adler32(checksum, mem + offset, next_stop_read - offset);
+		offset = next_stop_read + sizeof(void *); /* skip over the pointer */
+
+		uint32_t repetitions = skip_list_entry->repetitions;
+		const uint32_t checked_area_size = skip_list_entry->checked_area_size;
+		while (repetitions != 0) {
+			checksum = zend_adler32(checksum, mem + offset, checked_area_size);
+			offset += checked_area_size + sizeof(void *); /* skip over the pointer */
+			repetitions--;
+		}
+
+		skip_list_entry++;
+	}
+
+	if (size > offset) {
+		checksum = zend_adler32(checksum, mem + offset, size - offset);
+	}
+
 	return checksum;
 }

ext/opcache/zend_accelerator_util_funcs.h

@@ -35,7 +35,7 @@ zend_op_array* zend_accel_load_script(zend_persistent_script *persistent_script,
 
 #define ADLER32_INIT 1     /* initial Adler-32 value */
 
-unsigned int zend_adler32(unsigned int checksum, unsigned char *buf, uint32_t len, uint32_t *skip_list);
+unsigned int zend_adler32(unsigned int checksum, unsigned char *buf, uint32_t len);
 
 unsigned int zend_accel_script_checksum(zend_persistent_script *persistent_script);
 

ext/opcache/zend_file_cache.c

@@ -1030,11 +1030,8 @@ int zend_file_cache_script_store(zend_persistent_script *script, int in_shm)
 	}
 	zend_shared_alloc_destroy_xlat_table();
 
-	zend_string *const s = (zend_string*)ZCG(mem);
-
-	uint32_t zero = 0;
-	info.checksum = zend_adler32(ADLER32_INIT, buf, script->size, &zero);
-	info.checksum = zend_adler32(info.checksum, (unsigned char*)ZSTR_VAL(s), info.str_size, &zero);
+	info.checksum = zend_adler32(ADLER32_INIT, buf, script->size);
+	info.checksum = zend_adler32(info.checksum, (unsigned char*)ZSTR_VAL((zend_string*)ZCG(mem)), info.str_size);
 
 #if __has_feature(memory_sanitizer)
 	/* The buffer may contain uninitialized regions. However, the uninitialized parts will not be
@@ -1778,9 +1775,8 @@ zend_persistent_script *zend_file_cache_script_load(zend_file_handle *file_handl
 	close(fd);
 
 	/* verify checksum */
-	uint32_t zero = 0;
 	if (ZCG(accel_directives).file_cache_consistency_checks &&
-	    (actual_checksum = zend_adler32(ADLER32_INIT, mem, info.mem_size + info.str_size, &zero)) != info.checksum) {
+	    (actual_checksum = zend_adler32(ADLER32_INIT, mem, info.mem_size + info.str_size)) != info.checksum) {
 		zend_accel_error(ACCEL_LOG_WARNING, "corrupted file '%s' excepted checksum: 0x%08x actual checksum: 0x%08x\n", filename, info.checksum, actual_checksum);
 		zend_file_cache_unlink(filename);
 		zend_arena_release(&CG(arena), checkpoint);