Criterion icon indicating copy to clipboard operation
Criterion copied to clipboard

criterion parametrized tests fail in case of randomize_va_space

Open bazsi opened this issue 7 years ago • 6 comments

I've recently upgraded my system to Ubuntu zesty, and my parametrized tests fail with a segfault.

It seems that the parameters I receive get a corrupted pointer, at least if I am using randomize_va_space.

My compound type is:

struct findcrlf_params
{
  gchar *msg;
  gsize msg_len;
  gsize eom_ofs;
};

It is initialized with static:

  static struct findcrlf_params params[] =
  {
    { "a\nb\nc\n",  6,  1 },
    { "ab\nb\nc\n",  7,  2 },

But when I get to the test function, the "msg" pointer is invalid:

With randomize_va_space turned on:

Thread 1 "test_findcrlf" hit Breakpoint 1, findcrlf_test_impl (params=0x7ffff7fae280) at /home/bazsi/zwa/projects/syslog-ng-ose-3.6/source/syslog-ng/tests/unit/test_findcrlf.c:128
128	  gchar *eom = find_cr_or_lf(params->msg, params->msg_len);
(gdb) p params
$1 = (struct findcrlf_params *) 0x7ffff7fae280
(gdb) p *params
$2 = {msg = 0x55b8bc6122ef <error: Cannot access memory at address 0x55b8bc6122ef>, msg_len = 6, eom_ofs = 1}

With randomize_va_space turned off:

Thread 1 "test_findcrlf" hit Breakpoint 1, findcrlf_test_impl (params=0x7ffff7fae280) at /home/bazsi/zwa/projects/syslog-ng-ose-3.6/source/syslog-ng/tests/unit/test_findcrlf.c:128
128	  gchar *eom = find_cr_or_lf(params->msg, params->msg_len);
(gdb) p params
$1 = (struct findcrlf_params *) 0x7ffff7fae280
(gdb) p *params
$2 = {msg = 0x5555555562ef "a\nb\nc\n", msg_len = 6, eom_ofs = 1}

My criterion version is:

 ii  criterion-dev                             2.3.2-3-ubuntu1~zesty1    amd64                     Criterion library development files

bazsi avatar May 12 '17 10:05 bazsi

Do you perhaps have a self-contained .c file that I can just compile and debug on my end? I think BoxFort might be missing some corner-case ASLR there.

Snaipe avatar May 12 '17 10:05 Snaipe

This tiny program crashes for me nicely:

#include <criterion/criterion.h>
#include <criterion/parameterized.h>

struct findcrlf_params
{
  char *msg;
  int msg_len;
  int eom_ofs;
};

ParameterizedTestParameters(findcrlf, test)
{
  static struct findcrlf_params params[] =
  {
    { "a\nb\nc\n",  6,  1 },
  };

  return cr_make_param_array(struct findcrlf_params, params, sizeof (params) / sizeof(struct findcrlf_params));
}

ParameterizedTest(struct findcrlf_params *params, findcrlf, test)
{
  cr_expect(params->msg[0] != 0);
}


I compiled with: $ gcc criterion-testcase.c -lcriterion

bazsi avatar May 12 '17 15:05 bazsi

I'm starting to understand the issue.

Criterion is behaving correctly: it transfers your parameters verbatim as they are specified in ParameterizedTestParameters to your test function. Unfortunately, this means passing the address of "a\nb\nc\n" as-is, to the new process, which due to ASLR will have a different address space layout.

I'm afraid we can't do much here. Criterion could try to scan the array and correct the pointers, but we can't really assume that we have pointer values and not some really large integers that could refer to some memory. Hopefully we can address this in #204 with tagged parameters.

In the mean time you have two solutions: either disable ASLR, as you already did, or use cr_malloc to notify criterion that the object have to stay at the same address:

char *cr_strdup(const char *str) {
    char *out = cr_malloc(strlen(str) + 1);
    assert(out);
    strcpy(out, str);
    return out;
}

void free_params(struct criterion_test_params *crp)
{
    for (size_t i = 0; i < crp->length; ++i) {
        struct findcrlf_params *p = (struct findcrlf_params *) crp->params + i;
        cr_free(p->msg);
    }
}

ParameterizedTestParameters(findcrlf, test)
{
  static struct findcrlf_params params[] =
  {
    { cr_strdup("a\nb\nc\n"),  6,  1 },
  };
  return cr_make_param_array(struct findcrlf_params, params, sizeof (params) / sizeof(struct findcrlf_params), free_params);
}

Snaipe avatar May 12 '17 16:05 Snaipe

Hmm, how does aslr work through forks? Or critetion also execs itself?

On May 12, 2017 6:16 PM, "Franklin Mathieu" [email protected] wrote:

I'm starting to understand the issue.

Criterion is behaving correctly: it transfers your parameters verbatim as they are specified in ParameterizedTestParameters to your test function. Unfortunately, this means passing the address of "a\nb\nc\n" as-is, to the new process, which due to ASLR will have a different address space layout.

I'm afraid we can't do much here. Criterion could try to scan the array and correct the pointers, but we can't really assume that we have pointer values and not some really large integers that could refer to some memory. Hopefully we can address this in #204 https://github.com/Snaipe/Criterion/issues/204 with tagged parameters.

In the mean time you have two solutions: either disable ASLR, as you already did, or use cr_malloc to notify criterion that the object have to stay at the same address:

char *cr_strdup(const char *str) { char *out = cr_malloc(strlen(str) + 1); assert(out); strcpy(out, str); return out; } void free_params(struct criterion_test_params *crp) { for (size_t i = 0; i < crp->length; ++i) { struct findcrlf_params *p = (struct findcrlf_params *) crp->params + i; cr_free(p->msg); } } ParameterizedTestParameters(findcrlf, test) { static struct findcrlf_params params[] = { { cr_strdup("a\nb\nc\n"), 6, 1 }, }; return cr_make_param_array(struct findcrlf_params, params, sizeof (params) / sizeof(struct findcrlf_params), free_params); }

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Snaipe/Criterion/issues/208#issuecomment-301120787, or mute the thread https://github.com/notifications/unsubscribe-auth/AArldi7zjsnLRE7hp7mjV6X2tCvoXhpCks5r5IXwgaJpZM4NZHT7 .

bazsi avatar May 12 '17 16:05 bazsi

Criterion does exec itself (actually it would be more accurate to say that BoxFort does). It didn't use to be true, but we had a lot of problems with libraries like nanomsg that deadlocked when used across forks; calls to fork() had to be forbidden inside tests; memory leaks from the runner propagated to the inside of tests; we had to be consistent with windows where fork() semantics did not exist, ... and so on.

Snaipe avatar May 12 '17 16:05 Snaipe

Marking this as wontfix for the moment. I'll leave the issue open to remind me to find something more intuitive for #204.

Snaipe avatar May 13 '17 22:05 Snaipe