sancov_cmp.c always uses 1-byte value profiles

[ammaraskar]

In the commit 4e54182#diff-7aadefadeb1ef4ea6af462b15077e3bb06bfd52f42ff6c8f8ef534b64b748638L25-L37
sancov_cmp.c was changed to refactor a bunch of individual

  uintptr_t k = RETADDR;
  k = (k >> 4) ^ (k << 8);

#ifdef SANCOV_VALUE_PROFILE
  k &= CMP_MAP_SIZE - 1;
  __libafl_targets_value_profile2(k, arg1, arg2);
#endif

calls into

HANDLE_SANCOV_TRACE_CMP(2, arg1, arg2, 0);

#define HANDLE_SANCOV_TRACE_CMP(arg_size, arg1, arg2, arg1_is_const) { \
  uintptr_t k = RETADDR; \
  k = (k >> 4) ^ (k << 8); \
  SANCOV_VALUE_PROFILE_CALL(k, arg_size, arg1, arg2, arg1_is_const) \
  SANCOV_CMPLOG_CALL(k, arg_size, arg1, arg2, arg1_is_const) \
}

  #define SANCOV_VALUE_PROFILE_CALL(k, arg_size, arg1, arg2, arg1_is_const) \
    k &= CMP_MAP_SIZE - 1; \
    __libafl_targets_value_profile1(k, arg1, arg2);

Notice that the new macro SANCOV_VALUE_PROFILE_CALL only ever does __libafl_targets_value_profile1 and does not use arg_size meaning all value profiles end up using the 1-byte value profiles.

/cc @DanBlackwell

[DanBlackwell]

Good spot - I messed up that macro. I think it should have been:

#define HANDLE_SANCOV_TRACE_CMP(arg_size, arg1, arg2, arg1_is_const)           \
  {                                                                            \
    uintptr_t k = RETADDR;                                                     \
    k = (k >> 4) ^ (k << 8);                                                   \
    SANCOV_VALUE_PROFILE_CALL(k, arg_size, arg1, arg2, arg1_is_const)          \
    SANCOV_CMPLOG_CALL(k, arg_size, arg1, arg2, arg1_is_const)                 \
  }

#define SANCOV_VALUE_PROFILE_CALL(k, arg_size, arg1, arg2, arg1_is_const)      \
  k &= CMP_MAP_SIZE - 1;                                                       \
  __libafl_targets_value_profile##arg_size(k, arg1, arg2);

Note the ##arg_size to paste the arg_size in. Running clang -E file.c, where file.c is:

#define HANDLE_SANCOV_TRACE_CMP(arg_size, arg1, arg2, arg1_is_const)           \
  {                                                                            \
    uintptr_t k = RETADDR;                                                     \
    k = (k >> 4) ^ (k << 8);                                                   \
    SANCOV_VALUE_PROFILE_CALL(k, arg_size, arg1, arg2, arg1_is_const)          \
    SANCOV_CMPLOG_CALL(k, arg_size, arg1, arg2, arg1_is_const)                 \
  }

#define SANCOV_VALUE_PROFILE_CALL(k, arg_size, arg1, arg2, arg1_is_const)      \
  k &= CMP_MAP_SIZE - 1;                                                       \
  __libafl_targets_value_profile##arg_size(k, arg1, arg2);

HANDLE_SANCOV_TRACE_CMP(1, arg1, arg2, 0);
HANDLE_SANCOV_TRACE_CMP(2, arg1, arg2, 0);
HANDLE_SANCOV_TRACE_CMP(4, arg1, arg2, 0);
HANDLE_SANCOV_TRACE_CMP(8, arg1, arg2, 0);

Gives the output:

# 1 "macro.c"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 424 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "macro.c" 2
# 14 "macro.c"
{ uintptr_t k = RETADDR; k = (k >> 4) ^ (k << 8); k &= CMP_MAP_SIZE - 1; __libafl_targets_value_profile1(k, arg1, arg2); SANCOV_CMPLOG_CALL(k, 1, arg1, arg2, 0) };
{ uintptr_t k = RETADDR; k = (k >> 4) ^ (k << 8); k &= CMP_MAP_SIZE - 1; __libafl_targets_value_profile2(k, arg1, arg2); SANCOV_CMPLOG_CALL(k, 2, arg1, arg2, 0) };
{ uintptr_t k = RETADDR; k = (k >> 4) ^ (k << 8); k &= CMP_MAP_SIZE - 1; __libafl_targets_value_profile4(k, arg1, arg2); SANCOV_CMPLOG_CALL(k, 4, arg1, arg2, 0) };
{ uintptr_t k = RETADDR; k = (k >> 4) ^ (k << 8); k &= CMP_MAP_SIZE - 1; __libafl_targets_value_profile8(k, arg1, arg2); SANCOV_CMPLOG_CALL(k, 8, arg1, arg2, 0) };

Which looks a bit better to me. I haven't touched this codebase in a long time, so you'll need to double check that is doing what you'd expect.

[DanBlackwell]

Hey @ammaraskar, I'm guessing you spotted this because you've just been looking closely at the code. If I open a PR with that proposed fix from above are you comfortable to check that the logic is correct?

[ammaraskar]

Yup, I did a code review after finding that some longer magic numbers weren't being found by the libfuzzer afllib shim, I'm happy to review a change.

Longer term I'm planning on making a PR with some integration tests for the libfuzzer shim that should hopefully catch issues like this.

[DanBlackwell]

Hi @ammaraskar, #3095 has the proposed fix in it - if you can rerun your test subject that was misbehaving before and see if it's beahving now, that would be super helpful. A regression test would be awesome if you can contribute one!