Clang address sanitizer detects buffer overflow when mpc_err_print is used about mpc HOT 7 CLOSED

Thanks a lot for the error report. I'll check it out as soon as I get time.

from mpc.

Just dropping by to say that using gcc 4.9 on OS X 10.10, I can not reproduce the bug.

from mpc.

Hey,

Can you verify if this is fixed in 9481c65?

Definitely one of the more perculiar bugs I've come across.

I managed to reproduce something similar in Ubuntu using gcc. It seems that the call to calloc in mpcf_escape_new was getting optimised away into something odd and making accessing the pointer cause some kind of address violation.

First I tried switching calloc to malloc and zeroing the memory manually. This appeared to fix the issue on my side. I then tried just switching the variable declaration order so that calloc was called last - which also appeared to fix the issue.

It seems very weird that switching the variable declaration order would fix this problem (it shouldn't matter), so it may be an obscure bug in the compiler - because I couldn't spot anything obviously wrong with my code.

Thanks,

Dan

from mpc.

Hi Dan,

I'm afraid it doesn't seem to be fixed. However, I can't reproduce the bug with gcc, and valgrind doesn't find anything (regardless of whether I use gcc or clang) so it's quite possible this is a false positive on the part of clang's address sanitizer.

That said, I did a bit of primitive printf tracing and have found that the problem (if indeed it is a problem) occurs in a call to vsprintf in mpc_err_string_cat, to be specific:

void mpc_err_string_cat(char *buffer, int *pos, int *max, char const *fmt, ...) {
  /* TODO: Error Checking on Length */
  int left = ((*max) - (*pos));
  va_list va;
  va_start(va, fmt);
  if (left < 0) { left = 0;}
  (*pos) += vsprintf(buffer + (*pos), fmt, va);  // Clang detects problem here
  va_end(va);
}

At this point in the program, the argument fmt has been produced by mpc_err_char_unescape. Adding a null-terminator to this string prevents the address sanitizer from complaining:

static char char_unescape_buffer[4];  // Used to be [3]

static const char *mpc_err_char_unescape(char c) {

  char_unescape_buffer[0] = '\'';
  char_unescape_buffer[1] = ' ';
  char_unescape_buffer[2] = '\'';
  char_unescape_buffer[3] = '\0';  // Line added

  switch (c) {

    case '\a': return "bell";
    case '\b': return "backspace";
    case '\f': return "formfeed";
    case '\r': return "carriage return";
    case '\v': return "vertical tab";
    case '\0': return "end of input";
    case '\n': return "newline";
    case '\t': return "tab";
    case ' ' : return "space";
    default:
      char_unescape_buffer[1] = c;
      return char_unescape_buffer;
  }

}

I'm sorry that I have no idea whether this change breaks any other functionality in the library!

Cheers,
Simon

from mpc.

Oh that looks like a very real bug in the mpc_err_char_unescape function! Nice catch.

I'll make the update and merge it in. My suspicion is that gcc has some different vsprintf implementation that doesn't rely on a null terminated format string but that maybe was screwing with some memory elsewhere due to no null terminator and causing the weird calloc bug. I'll test it and see.

from mpc.

Please verify if this is fixed in d6347af and thanks again.

from mpc.

I can confirm that this is fixed in d6347af. Thanks, this discussion has been educational for me!

from mpc.