Proper usage of malloc() and free() in c

Question

I'm new to C so forgive me if this is too obvious, but I am having an issue finding the error in my code that is leading to a segmentation fault. I believe the issue may be in the usage of malloc(), but I am not positive.

Here is the code:

#include        <stdlib.h>
#include        <stdio.h>
#include        <string.h>

#define         MAX_STRING      20

char*   getFirstName    (char*  firstName
                        )
{
  char* myfirstName = (char*)malloc(strlen(firstName)+1);
  printf("Please enter your first name: ");
  fgets(firstName,MAX_STRING,stdin);
  return(myfirstName);
}


char*   getLastName     (char* lastName
                    )
{
  char* mylastName = (char*)malloc(strlen(lastName)+1);

  printf("Please enter your last name: ");
  fgets(lastName,MAX_STRING,stdin);
  return(mylastName);
}


char*   getNickName     (char*  nickName
                        )
{
  char* mynickName = (char*)malloc(strlen(nickName)+1);
  printf("Please enter your nick name: ");
  fgets(nickName,MAX_STRING,stdin);
  return(mynickName);
}


char*   getCompleteName (const char*    firstName,
                         const char*    lastName,
                         const char*    nickName,
                         char*          completeName
                        )
{
  snprintf(completeName,MAX_STRING,"%s \"%s\"    %s",firstName,nickName,lastName);
}


int     main    ()
{
  char*         firstName;
  char*         lastName;
  char*         nickName;
  char*         completeName;

  firstName     = getFirstName(firstName);
  lastName      = getLastName(lastName);
  nickName      = getNickName(nickName);

  completeName  = getCompleteName(firstName,lastName,nickName,completeName);
  printf("Hello %s.\n",completeName);
 free(firstName);
 free(lastName);
 free(nickName);
 return(EXIT_SUCCESS);
}

Does it seem that I am using malloc() in the correct way?

Answer 1

The functions you have written to enter data do not use their parameters (or they use them incorrectly). Thus, it makes no sense to declare them like:

char*   getFirstName    (char*  firstName );

Within the functions, memory is allocated and a pointer to the memory is returned.

Moreover, this statement:

char* myfirstName = (char*)malloc(strlen(firstName)+1);

is invalid. The argument for parameter firstName was not initialized and does not point to any string.

Or you try to allocate memory and save the corresponding address in variable myfirstName:

char* myfirstName = (char*)malloc(strlen(firstName)+1);

but then try to read data using pointer firstName:

fgets(firstName,MAX_STRING,stdin);

The function getCompleteName is also invalid. Again, there was no allocated memory that should be pointed to by completeName where you try to concatenate other strings. And the function returns nothing.

char*   getCompleteName (const char*    firstName,
                         const char*    lastName,
                         const char*    nickName,
                         char*          completeName
                        )
{
  snprintf(completeName,MAX_STRING,"%s \"%s\"    %s",firstName,nickName,lastName);
}

Take into account that function fgets also includes in the target array the new line character.

Thus, correct functions can look like the following definition below:

char*   getFirstName()
{
    char* myfirstName = ( char* )malloc( MAX_STRING );

    printf( "Please enter your first name: " );

    fgets( myfirstName, MAX_STRING, stdin );

    size_t n = strlen( myfirstName );

    if ( n != 0 && myfirstName[n-1] == '\n' ) myfirstName[n-1] = '\0';

    return myfirstName;
}

and:

char*   getCompleteName (const char*    firstName,
                         const char*    lastName,
                         const char*    nickName,
                        )
{
    const char *format = "%s \"%s\"    %s";

    size_t n = strlen( firstName ) + strlen( lastName ) + 
               strlen( nickName ) + strlen( format );

    completeName = ( char * )malloc( n );

    snprintf( completeName, n, format, firstName,nickName,lastName);

    return completeName;
}

Define other functions in similar ways.

Answer 2

The preferred way to write a malloc call is

T *p = malloc( N * sizeof *p );

calloc is similar:

T *p = calloc( N, sizeof *p );

The cast is unnecessary, and under C89 compilers can mask a bug.

The problem with this call

  char* myfirstName = (char*)malloc(strlen(firstName)+1);

is the that firstName parameter hasn't been initialized; it doesn't point to a string, so calling strlen on it is undefined. In this case, you should use the MAX_STRING constant instead:

char *myFirstName = malloc( (MAX_STRING + 1) * sizeof *myFirstName );

In this case, sizeof *myFirstName is redundant (sizeof (char) is 1 by definition), but it doesn't hurt anything, and if you decide to change the type of myFirstName to wchar * for some crazy reason, the call will still work properly.

Answer 3

char* myfirstName = (char*)malloc(strlen(firstName)+1);

In the above line, you're using firstName uninitialized. Strlen(firstName) will only work when firstName has some length which you passed to the function without giving it. Same goes to lastName and nickName.

Answer 4

You should not use strlen() within malloc() to determine the number of bytes, you want to allocate into memory. Since, the character pointer variable, you are specifying within strlen() are "firstName,nickName,lastName", which are storing addresses of unknown location, which causes the given error. Therefore You should specify number of bytes, you want to allocate for different character pointer variable explicitly like this:

char* mylastName = (char*)malloc(150);

Here, 150 bytes will be allocated into memory and reference will be set char* mylastname .

Answer 5

I'm also new, but I think your problem is here:

char*         firstName;

firstName     = getFirstName(firstName);

char* myfirstName = (char*)malloc(strlen(firstName)+1);

you are implementing strlen on uninitialize char pointer. You have to specify a max len (#define MAX_SIZE 64) and use it as you do not know the length of the names.

Also consider this, the first 3 functions does the same, you should consider having one function instead.

Hope I helped