Double free() occurrence where it should not

Question

I have a frustrating problem for which I can't find an answer to.

I have this function:

// Append character to the end of a string
void str_AppendChar(char* s, const char* ch)
{
  // +2 because of 2x '\0'
  char* buff = malloc(strlen(s)+strlen(ch)+2);
  memset(buff, 0, sizeof(buff));

  // Copy the whole string in buff
  strcpy(buff, s);

  // Append ch at the end of buff
  int len = strlen(buff);
  strcpy(buff+len, ch);

  // Set end of the string
  *(buff+strlen(buff)-3) = '\0';

  strcpy(s, buff);

  free(buff);
}

In which for some reason my program tries to execute free two time at the end.

The code in which I use AppendChar() is:(a bit ugly but bear with me)

void it_GatherCmd(cmd_Details* lineptr[], char* cmd)
{
  // Used to count number of rows in lineptr
  int nlines;

  Detailptr p;
  char ch;
  char* word = (char*)malloc(sizeof(char)+256);
  memset(word, 0, sizeof(word));

  nlines = 0;
  while ((ch = *cmd++) != '\n')
  {
      if (ch != ' ' && ch != '\0' )
          str_AppendChar(word, &ch);
      else
      {
          int type = dict_CheckWord(word);

          if (type != -1)
          {
              p = it_CopyInfo(word, type);
              lineptr[nlines++] = p;
          }
          memset(word, 0, sizeof(word));
      }
   }
   //EDIT*
   free(word);
}

And my main:

int main()
{
   cmd_Details* arrCmd[MAXLINES];
   char* str = "just some string";

   it_GatherCmd(arrCmd, str);

   printf("%s", str);
   return 0;
}

AppendChar() was working without problems until I created it_GetCharCmd() and used it there. I've spent around 3 hours on this and I can't find the problem. Did some searching on the internet but the things I have found didn't exactly relate to my problem.

I'm pretty sure that `memset` call in both functions will not work as you expect; `sizeof(buf)` here will be `sizeof(char*)`, not the length of the array. — user4520, May 16 '15 at 10:08
For `strcpy(s, buff);` I think there won't be a problem because the data that is going to be used will most likely never reach `sizeof(char)+256`. But I'll keep in mind to optimize it later. `memset` is used to clear the data in the allocated memory so it can be reused later. After the work is finished the memory is freed. — B Mutev, May 16 '15 at 10:19
The whole procedure is very ineffective. Why do you allocate and free memory locally when all you do is append a char to a word? I think (with the exception of storing the required data in your command structure), you don't need dynamic allocation at all. For example a 256-byte buffer is best created as automatic memory on the steck with `char buf[256]` and you're done. — M Oehm, May 16 '15 at 10:28
Not necessarily the cause of the specific problem, but in `str_AppendChar()` the second parameter is treated like a string (used with `strlen()` and `strcpy()`), while it really just points to a single character, not a null-terminated sequence of characters. So it will try to access the random memory that follows the character, up to the next null it happens to find. — sth, May 16 '15 at 10:28
Here `strcpy(buff+len, ch);` a simple `strcat(buff, ch);` would do. — alk, May 16 '15 at 10:33
`memset(buff, 0, sizeof(buff));` - huh? do you know what `sizeof` does? — Karoly Horvath, May 16 '15 at 10:37

user4520 · Accepted Answer · 2015-05-16T11:52:54.583

There are a few problems with this code.

Firstly, if str_AppendChar actually appends a single character as its name implies, why are you giving it a const char* which implies a C string? There's zero gain passing a pointer instead of the actual object here, as would be the case with some structs; effectively you still need to push 4 bytes to the stack.

Secondly, as I have pointed out in the comments, the problem is that you don't initialize the allocated buffer properly - sizeof(buff) returns well, the size of buff, and buff being a char* this is most likely 4. Whilst simply changing sizeof(buff) to strlen(s)+strlen(ch)+2, which is how much memory you have actually allocated fixed the problem (and since sizeof(buff) might have been more than what you have actually allocated, you were writing past that memory), I'd suggest simplifying the function like this:

// Append character to the end of a string
void str_AppendChar(char* s, char ch)
{
    size_t sLen = strlen(s);
    char* buff = (char*)malloc(sLen + 2);       // 1 for the appended char, 1 for \0
    //memset(buff, 0, sLen + 2);   //not necessary, we'll overwrite the memory anyway

    // Copy the whole string in buff
    strcpy(buff, s);

    // append our char and null-terminate
    buff[sLen] = ch;
    buff[sLen + 1] = '\0';

    strcpy(s, buff);

    free(buff);
}

Note that this code is still bad; it happily assumes that s will be large enough to hold that one extra character, which isn't always the case.

Also regarding your it_gatherCmd function; it should take a const char* since it doesn't modify it in any way (and indeed, the way you're calling it, you're giving it a const char*; modifying a string literal is undefined behavior, on Windows you'd probably crash due to violating page permission).

@KarolyHorvath True, Windows will zero it for you, but I feel safer doing it. — user4520, May 16 '15 at 10:40
@szczurcio: That's not thze reason: You'll overwrite it immediately after setting it to zero, so it's unnecessary. — M Oehm, May 16 '15 at 10:44
Thank you for your time. Indeed I didn't do things right, with your version it works. I'll be more careful from now on. — B Mutev, May 16 '15 at 10:47
The part "`malloc` returns `void*` and C won't let you initialize a `char*` with a `void*`, you need to cast it. If this compiles for you, you're likely compiling for C++." is backwards and wrong. It's legal C, but illegal C++. [Don't cast return value of `malloc` in C.](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — milleniumbug, May 16 '15 at 11:16
@milleniumbug Oops, right, it should've been the other way round. Thanks for the correction. — user4520, May 16 '15 at 11:52

score 0 · Answer 2 · answered May 16 '15 at 10:59

As far as I can see, you are building a string successively while scanning the command. There is absolutely no need to copy the string to append to twice when appending a char. What you are effectively doing, is:

void str_AppendChar(char* s, char ch)
{
    int len = strlen(buff);

    s[len++] = ch;
    s[loen] = '\0';
}

Note that you determine the string length with strlen every time, which will traverse the whole string. Even worse, you don't have any information on the maximum avaiilable buffer size, so despite all your allocating, copying and freeing, the original string might overflow.

Instead of

str_AppendChar(word, &ch);

use a local buffer in automatic memory:

char word[20];
int wlen = 0;

and append just like this:

if (wlen + 1 < sizeof(word)) word[wlen++] = *cmd;

This will leave the word buffer unterminated, so before using it, append it:

word[wlen] = '\0';
printf("next word: '%s'\n", word);

(Alternatively you could ensure that the string is null-terminated at all times, of course.)

When you reset the buffer for a fresh word, there's no need to memset the whole buffer; just reset wlen to zero.

Double free() occurrence where it should not

2 Answers2