Setting counting_illegal_callback may hide failing tests

[jonasnick]

The following code snippet very roughly resembles what happens in the libsecp test harness and illustrates the issue:

/* in the tests we have access to a global context */
static secp256k1_context *ctx = NULL;

void test_foo() {
  int ecount;
  context_set_error_callback(ctx, counting_illegal_callback_fn, &ecount);
  /* do some tests with ecount */
}
void test_bar() {
  /* we don't set the counting_illegal_callback here because we don't want to test that here */
  some_function(ctx);
}
void main() {
  test_foo();
  test_bar();
}

The code is fine, until one day some_function(ctx) results in the illegal callback being called.
Then we'd want the test to fail but instead what happens is that some stack region formerly known as ecount is modified, which does not necessarily result in a crash.

One solution would be to never add the counting_illegal_callback to the global context and instead create a local context for counting.

[real-or-random]

One solution would be to never add the counting_illegal_callback to the global context and instead create a local context for counting.

Or make the ecount global. 🤷 I guess that's good enough for test code.

[apoelstra]

We could write an EXPECT_ARG_CHECK_FAIL macro (or maybe something with a nicer name :)) which creates a fresh ecount variable at 0, sets the context object's error callbacks, checks the ecount is 1, then restores the old callbacks.

This would also get rid of all the ecount-accounting code, which is quite a PITA to maintain and sometimes results in huge diffs.

[sipa]

We could write an EXPECT_ARG_CHECK_FAIL macro (or maybe something with a nicer name :)) which creates a fresh ecount variable at 0, sets the context object's error callbacks, checks the ecount is 1, then restores the old callbacks.

@apoelstra That would be so much more readable!

[real-or-random]

We have CHECK_ILLEGAL and CHECK_ILLEGAL_VOID since e39d954:

secp256k1/src/tests.c

Lines 55 to 72 in c545fdc

	/* TODO Use CHECK_ILLEGAL(_VOID) everywhere and get rid of the uncounting callback */
	/* CHECK that expr_or_stmt calls the illegal callback of ctx exactly once
	*
	* For checking functions that use ARG_CHECK_VOID */
	#define CHECK_ILLEGAL_VOID(ctx, expr_or_stmt) do { \
	int32_t _calls_to_illegal_callback = 0; \
	secp256k1_callback _saved_illegal_cb = ctx->illegal_callback; \
	secp256k1_context_set_illegal_callback(ctx, \
	counting_illegal_callback_fn, &_calls_to_illegal_callback); \
	{ expr_or_stmt; } \
	ctx->illegal_callback = _saved_illegal_cb; \
	CHECK(_calls_to_illegal_callback == 1); \
	} while(0);
	/* CHECK that expr calls the illegal callback of ctx exactly once and that expr == 0
	*
	* For checking functions that use ARG_CHECK */
	#define CHECK_ILLEGAL(ctx, expr) CHECK_ILLEGAL_VOID(ctx, CHECK((expr) == 0))

The "only" thing left to do is to use these everywhere (and maybe introduce a similar macro for the error callback).