crcsync byte-at-a-time test reveals flaws in buffer handling.

(Reported by Alex Wulms <alex.wulms@scarlet.be>)

ccan/crcsync/crcsync.c

@@ -138,6 +138,7 @@ size_t crc_read_block(struct crc_context *ctx, long *result,
 		goto have_match;
 	}
 
+	/* old is the trailing edge of the checksum window. */
 	if (buffer_size(ctx) >= ctx->block_size)
 		old = ctx->buffer + ctx->buffer_start;
 	else
@@ -153,6 +154,7 @@ size_t crc_read_block(struct crc_context *ctx, long *result,
 						    *old, *p,
 						    ctx->uncrc_tab);
 			old++;
+			/* End of stored buffer?  Start on data they gave us. */
 			if (old == ctx->buffer + ctx->buffer_end)
 				old = buf;
 		} else {
@@ -167,11 +169,6 @@ size_t crc_read_block(struct crc_context *ctx, long *result,
 		p++;
 	}
 
-	/* Make sure we have a copy of the last block_size bytes.
-	 * First, copy down the old data.  */
-	if (buffer_size(ctx)) {
-	}
-
 	if (crcmatch >= 0) {
 		/* We have a match! */
 		if (ctx->literal_bytes > ctx->block_size) {
@@ -187,12 +184,15 @@ size_t crc_read_block(struct crc_context *ctx, long *result,
 			assert(ctx->literal_bytes == 0);
 			ctx->have_match = -1;
 			ctx->running_crc = 0;
+			/* Nothing more in the buffer. */
+			ctx->buffer_start = ctx->buffer_end = 0;
 		}
 	} else {
 		/* Output literal if it's more than 1 block ago. */
 		if (ctx->literal_bytes > ctx->block_size) {
 			*result = ctx->literal_bytes - ctx->block_size;
-			ctx->literal_bytes = ctx->block_size;
+			ctx->literal_bytes -= *result;
+			ctx->buffer_start += *result;
 		} else
 			*result = 0;
 
@@ -243,29 +243,34 @@ static size_t final_block_match(struct crc_context *ctx)
 long crc_read_flush(struct crc_context *ctx)
 {
 	long ret;
+	size_t final;
 
-	/* In case we ended on a whole block match. */
-	if (ctx->have_match == -1) {
-		size_t final;
-
-		final = final_block_match(ctx);
-		if (!final) {
-			/* This is how many bytes we're about to consume. */
-			ret = buffer_size(ctx);
-			ctx->buffer_start += ret;
-			ctx->literal_bytes -= ret;
+	/* We might have ended right on a matched block. */
+	if (ctx->have_match != -1) {
+		ctx->literal_bytes -= ctx->block_size;
+		assert(ctx->literal_bytes == 0);
+		ret = -ctx->have_match-1;
+		ctx->have_match = -1;
+		ctx->running_crc = 0;
+		/* Nothing more in the buffer. */
+		ctx->buffer_start = ctx->buffer_end;
+		return ret;
+	}
 
-			return ret;
-		}
-		ctx->buffer_start += final;
-		ctx->literal_bytes -= final;
-		ctx->have_match = ctx->num_crcs-1;
+	/* Look for truncated final block. */
+	final = final_block_match(ctx);
+	if (!final) {
+		/* Nope?  Just a literal. */
+		ret = buffer_size(ctx);
+		ctx->buffer_start += ret;
+		ctx->literal_bytes -= ret;
+		return ret;
 	}
 
-	/* It might be a partial block match, so no assert */
-	ctx->literal_bytes = 0;
-	ret = -ctx->have_match-1;
-	ctx->have_match = -1;
+	/* We matched (some of) what's left. */
+	ret = -(ctx->num_crcs-1)-1;
+	ctx->buffer_start += final;
+	ctx->literal_bytes -= final;
 	return ret;
 }
 

ccan/crcsync/test/api.c

@@ -64,15 +64,36 @@ static void test_sync(const char *buffer1, size_t len1,
 		      const struct result results[], size_t num_results)
 {
 	struct crc_context *ctx;
-	size_t used, ret, i, curr_literal = 0;
+	size_t used, ret, i, curr_literal;
 	long result;
 	uint32_t crcs[num_blocks(len1, block_size)];
 
 	crc_of_blocks(buffer1, len1, block_size, 32, crcs);
+
+	/* Normal method. */
 	ctx = crc_context_new(block_size, 32, crcs, ARRAY_SIZE(crcs));
 
+	curr_literal = 0;
 	for (used = 0, i = 0; used < len2; used += ret) {
 		ret = crc_read_block(ctx, &result, buffer2+used, len2-used);
+		check_result(result, &curr_literal, results, num_results, &i);
+	}
+
+	while ((result = crc_read_flush(ctx)) != 0)
+		check_result(result, &curr_literal, results, num_results, &i);
+
+	check_finalized_result(curr_literal, results, num_results, &i);
+	
+	/* We must have achieved everything we expected. */
+	ok1(i == num_results);
+	crc_context_free(ctx);
+
+	/* Byte-at-a-time method. */
+	ctx = crc_context_new(block_size, 32, crcs, ARRAY_SIZE(crcs));
+
+	curr_literal = 0;
+	for (used = 0, i = 0; used < len2; used += ret) {
+		ret = crc_read_block(ctx, &result, buffer2+used, 1);
 
 		check_result(result, &curr_literal, results, num_results, &i);
 	}
@@ -93,7 +114,7 @@ int main(int argc, char *argv[])
 	unsigned int i;
 	uint32_t crcs1[12], crcs2[12];
 
-	plan_tests(733);
+	plan_tests(1454);
 
 	buffer1 = calloc(1024, 1);
 	buffer2 = calloc(1024, 1);