CODEWITHSUNDEEP
cstringmemory-managementstrcat
Damiano Barbati
Damiano Barbati

Reputation: 3496

Malloc() memory corruption error after concatenating a string

Guys I'm generating a string which rappresent a path to a file, concatenating a macro and a string. The function is this:

char *userPath(char *username)
{
   char *path = (char*)malloc(sizeof(char) * (strlen(MAILBOXES) + strlen(username) + 1));
   path[0] = '\0';
   strcat(path, MAILBOXES);
   strcat(path, "/");
   strcat(path, username);
   return path;
}

The returned pointer reference a correct string, but after some call to this function the process throws out a very very bad * glibc detected ./mmboxd: malloc(): memory corruption: 0x085310a8 ** with the relative backtrace. I know it's here the problem, since I started having this error once implemented it, and also because the only malloc I use is here. What's wrong with this piece of code?

Upvotes: 0

Views: 2524

Answers (8)

Chris Lutz
Chris Lutz

Reputation: 75399

Here's the problem:

   char *path = (char*)malloc(sizeof(char) * (strlen(MAILBOXES) + strlen(username) + 1));

You're allocating enough memory for a) all the characters in MAILBOXES, b) all the characters in username, and c) the '/' character, but you're forgetting d) the terminating '\0' character! So + 1 should be + 2

There are a few other oddities about your code, but they're not wrong, just things that could be better:

  1. You don't need to cast the return value of malloc in C, and some (like me) consider it bad style for various reasons that you're more than capable of Googling.
  2. sizeof(char) is always 1 (this is defined in the standard). Some people say to keep it in for symmetry. Some say take it out since it's one. Some say change it to sizeof *path, so that if you change path to a wchar_t * the malloc will correctly adjust to keep allocating the right size.
  3. Using strcat to write the first bit of data to a string is potentially inefficient. Why not drop the path[0] = '\0'; line and just use strcpy for the first bit of data?
  4. You calculate the lengths of all of the strings, but then you throw them away and use strcat, which will re-traverse the (previously calculated) lengths to find the right spot. If you stored the results of your two strlen calls, you wouldn't need to use strcat and unnecessarily keep recalculating where the end of the string is.
  5. Using strcat to append a single character is inefficient.
  6. You don't check the return value of malloc for success or failure before you use it.

Upvotes: 4

karlphillip
karlphillip

Reputation: 93410

Your +1 in the end of malloc() accounts for the /. But you need space for the null character at the end, which is added by strcat(). So it's a +2.

char *path = (char*)malloc(sizeof(char) * (strlen(MAILBOXES) + strlen(username) + 2));

Upvotes: 1

Joe
Joe

Reputation: 42627

You're not budgeting in a char for the terminating null. Your malloc length should be +2, not +1.

Upvotes: 1

Mikko Ohtamaa
Mikko Ohtamaa

Reputation: 83348

You need to allocate one extra byte for null character "\x00" as the string terminator in C strings.

Currently you allocate only one extra byte for / character.

So try +2 instead of +1

Upvotes: 1

ssegvic
ssegvic

Reputation: 3142

It appears you would need to malloc an another byte for zero termination.

Upvotes: 1

Max
Max

Reputation: 3180

When you allocate the "path" string you forgot to add the length of the "/" char that you add between MAILBOXES and username.

Upvotes: 1

David Heffernan
David Heffernan

Reputation: 612954

You don't appear to have allowed space for the zero-terminator. You should be allocating an extra char for that.

I'm assuming that the +1 in the malloc() is for the path separator. Make it +2 and you'll have space for the terminating null character.

Upvotes: 2

user2100815
user2100815

Reputation:

The +1 should be +2 to take into account the separator you add and the null terminator. And you can omit sizeof(char), which will always be 1.

Upvotes: 9

Related Questions