Merge pull request katef#496 from katef/sv/bugfix-compact-states-shou…

…ld-remap-endids

fsm_compact_states must remap endids, to avoid dangling references.

src/libfsm/endids.c

@@ -880,6 +880,10 @@ fsm_endid_iter_bulk(const struct fsm *fsm,
 		return 1;
 	}
 
+#ifndef NDEBUG
+	const fsm_state_t state_count = fsm_countstates(fsm);
+#endif
+
 	bucket_count = ei->bucket_count;
 
 	for (b_i = 0; b_i < bucket_count; b_i++) {
@@ -891,6 +895,7 @@ fsm_endid_iter_bulk(const struct fsm *fsm,
 
 		count = b->ids->count;
 
+		assert(b->state < state_count);
 		if (!cb(b->state, &b->ids->ids[0], count, opaque)) {
 			return 0;
 		}
@@ -1026,3 +1031,62 @@ fsm_increndids(struct fsm * fsm, int delta)
 	fsm_mapendids(fsm, incr_remap, &delta);
 }
 
+int
+fsm_endid_compact(struct fsm *fsm, fsm_state_t *mapping, size_t mapping_count)
+{
+	/* Don't reallocate unless something has actually changed. */
+	bool changes = false;
+	for (size_t i = 0; i < mapping_count; i++) {
+		if (mapping[i] != i) {
+			changes = true;
+			break;
+		}
+	}
+
+	/* nothing to do */
+	if (!changes) { return 1; }
+
+	struct endid_info *ei = fsm->endid_info;
+
+	struct endid_info_bucket *nbuckets = f_malloc(fsm->alloc,
+	    ei->bucket_count * sizeof(nbuckets[0]));
+	if (nbuckets == NULL) {
+		return 0;
+	}
+
+	const uint64_t mask = ei->bucket_count - 1;
+	assert((ei->bucket_count & mask) == 0);
+
+	/* initialize to empty */
+	for (size_t nb_i = 0; nb_i < ei->bucket_count; nb_i++) {
+		nbuckets[nb_i].state = BUCKET_NO_STATE;
+	}
+
+	for (size_t ob_i = 0; ob_i < ei->bucket_count; ob_i++) {
+		const struct endid_info_bucket *ob = &ei->buckets[ob_i];
+		if (ob->state == BUCKET_NO_STATE) { continue; }
+
+		assert(ob->state < mapping_count);
+		const fsm_state_t nstate = mapping[ob->state];
+		if (nstate == FSM_STATE_REMAP_NO_STATE) { continue; }
+
+		const uint64_t hash = hash_id(nstate);
+
+		bool placed = false;
+		for (size_t probes = 0; probes < ei->bucket_count; probes++) {
+			const size_t nb_i = (hash + probes) & mask;
+			struct endid_info_bucket *nb = &nbuckets[nb_i];
+			if (nb->state == BUCKET_NO_STATE) {
+				nb->state = nstate;
+				nb->ids = ob->ids;
+				placed = true;
+				break;
+			}
+		}
+		assert(placed);
+	}
+
+	f_free(fsm->alloc, ei->buckets);
+	ei->buckets = nbuckets;
+	return 1;
+}

src/libfsm/endids.h

@@ -63,4 +63,7 @@ fsm_endid_iter_state(const struct fsm *fsm, fsm_state_t state,
 void
 fsm_endid_dump(FILE *f, const struct fsm *fsm);
 
+int
+fsm_endid_compact(struct fsm *fsm, fsm_state_t *mapping, size_t mapping_count);
+
 #endif

src/libfsm/libfsm.syms

@@ -63,6 +63,7 @@ fsm_print_vmops_main
 fsm_clone
 fsm_free
 fsm_new
+fsm_new_statealloc
 fsm_move
 fsm_merge
 fsm_addstate

src/libfsm/state.c

@@ -18,6 +18,7 @@
 #include <adt/edgeset.h>
 
 #include "internal.h"
+#include "endids.h"
 
 int
 fsm_addstate(struct fsm *fsm, fsm_state_t *state)
@@ -179,7 +180,7 @@ fsm_compact_states(struct fsm *fsm,
 	const fsm_state_t orig_statecount = fsm->statecount;
 
 	fsm_state_t *mapping = f_malloc(fsm->alloc,
-	    fsm->statecount * sizeof(mapping[0]));
+	    orig_statecount * sizeof(mapping[0]));
 	if (mapping == NULL) {
 		return 0;
 	}
@@ -254,6 +255,10 @@ fsm_compact_states(struct fsm *fsm,
 		}
 	}
 
+	/* Remap end metadata */
+	if (!fsm_endid_compact(fsm, mapping, orig_statecount)) {
+		return 0;
+	}
 	assert(dst == kept);
 	assert(kept == fsm->statecount);
 

tests/endids/endids_trim_must_compact_endids.c

@@ -0,0 +1,64 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <assert.h>
+
+#include <fsm/fsm.h>
+#include <fsm/print.h>
+
+/* Regression: When fsm_trim called fsm_compact_states it previously
+ * didn't remap endids, so the endid collection could refer to stale
+ * state IDs or past the end of fsm->states[]. */
+int main(void)
+{
+	size_t limit = 100;
+	const fsm_end_id_t end_id = 12345;
+
+	/* Create FSMs with a bunch of garbage states, to increase
+	 * the likelihood that the endid's state not being updated
+	 * after calling fsm_trim triggers the bug. */
+	for (size_t end_state = 1; end_state < limit; end_state++) {
+		struct fsm *fsm = fsm_new(NULL);
+		assert(fsm != NULL);
+
+		for (size_t i = 0; i < limit; i++) {
+			if (!fsm_addstate(fsm, NULL)) {
+				return EXIT_FAILURE;
+			}
+		}
+
+		const fsm_state_t start_state = end_state - 1;
+		fsm_setstart(fsm, start_state);
+		fsm_setend(fsm, end_state, 1);
+
+		/* add an any edge, so the end state is reachable */
+		if (!fsm_addedge_any(fsm, start_state, end_state)) {
+			return EXIT_FAILURE;
+		}
+
+		if (!fsm_setendid(fsm, end_id)) {
+			return EXIT_FAILURE;
+		}
+
+		/* Call fsm_trim, as fsm_minimise would. */
+		(void)fsm_trim(fsm, FSM_TRIM_START_AND_END_REACHABLE, NULL);
+
+		/* Execute the fsm and check that the endid's state was updated. */
+		fsm_state_t end;
+		const char *input = "x";
+		int ret = fsm_exec(fsm, fsm_sgetc, &input, &end, NULL);
+		assert(ret == 1);
+
+		size_t count = fsm_endid_count(fsm, end);
+		assert(count == 1);
+
+		fsm_end_id_t id_buf[1];
+		if (!fsm_endid_get(fsm, end, 1, id_buf)) {
+			return EXIT_FAILURE;
+		}
+		assert(id_buf[0] == end_id);
+
+		fsm_free(fsm);
+	}
+	return EXIT_SUCCESS;
+}