C: How do I pass malloc'ed pointers into a function and free them?

Question

I'm working on a client-server program using sockets and threads. I have a menu for the client, and I'm trying to send a char from the client to the server, to then call the corresponding functions. I'm trying to make use of pointers and malloc but I don't think I'm quite understanding how to pass them and free them. I'm getting memory leaks and errors like:

./server': double free or corruption (fasttop): 0x00007f37dc000a50 ***

and

Segmentation fault

Here is my client menu:

char *choice = (char*) malloc(sizeof(char));

do {

    // get a string from the server
    get_msg(sockfd);

    printf("\nFile and Information System\n");
    printf("===========================\n");
    printf("1. Show IP and Student ID\n");
    printf("2. Display server time\n");
    printf("3. Display system information\n");
    printf("4. List files on server\n");
    printf("5. File Transfer\n");
    printf("6. Exit\n");
    printf("Enter choice: ");
    scanf("%s", choice);

    //Send input to server
    send_menu_choice(sockfd, choice);

    switch (*choice) 
    {
        case '1':
                get_ip(sockfd);
            break;
        case '2':
                get_time(sockfd);
            break;
        case '3':
                get_and_send_uname(sockfd);
            break;
        case '4':
                get_file_list(sockfd);
            break;
        case '5':
                printf("File transfer not yet implemented\n");;
            break;
        case '6':
                printf("Exiting...\n");
            break;
        default:
                printf("Invalid choice\n");
    }

    //if(menu_choice != NULL)
    //{
    //    free(menu_choice);
    //}

} while (*choice != '6');

if (choice != NULL)
{
    free(choice);
}

Send menu option to server:

void send_menu_choice(int socketNumber, char *choice)
{
    printf("Sending menu choice...\n");
    size_t n = strlen(choice) + 1;

    writen(socketNumber, (unsigned char *) &n, sizeof(size_t));
    writen(socketNumber, (unsigned char *) choice, n);

    printf("Sent choice: %s\n\n", choice);
}

Server side:

char *menu_choice = (char*) malloc(sizeof(char));

do { 

    printf("Waiting for client to select option...\n\n");
    send_msg(connfd);

    get_menu_choice(connfd, menu_choice);
    printf("Client %d choice was: %s\n", connfd, menu_choice);

    switch (*menu_choice) 
    {
        case '1':
                send_ip(connfd);
            break;
        case '2':
                send_time(connfd);
            break;
        case '3':
                get_and_send_uname(connfd, *uts);
            break;
        case '4':
                send_file_list(connfd);
            break;
        case '5':
                printf("File Transfer not implemented\n");
            break;
        case '6': 
            break;
        default:
                printf("Invalid choice\n");
    }

    //if(menu_choice != NULL)
    //{
    //    free(menu_choice);
    //}

} while (*menu_choice != '6');

if (choice != NULL)
{
    free(choice);
}

Get menu option from client:

void get_menu_choice(int socketNumber, char *choice)
{
    size_t n;

    readn(socketNumber, (unsigned char *) &n, sizeof(size_t));  
    readn(socketNumber, (unsigned char *) choice, n);

    printf("Received: %zu bytes\n\n", n);
}

---

Additional functions as requested:

Send msg (server side):

void send_msg(int socket)
{
    char msg_string[] = "\nPlease enter an option:";

    size_t n = strlen(msg_string) + 1;
    writen(socket, (unsigned char *) &n, sizeof(size_t));   
    writen(socket, (unsigned char *) msg_string, n);
}

Get msg (client side:

void get_msg(int socket)
{
    char msg_string[32];
    size_t k;

    readn(socket, (unsigned char *) &k, sizeof(size_t));    
    readn(socket, (unsigned char *) msg_string, k);

    printf("%s\n", msg_string);
}

What am I doing wrong here? am I not affecting the value of where the pointer is pointing to?

Answer 1

This is a generic answer, here are the functions I used on a project:

/* this is safe free() and no not what you asked, check next one */
void my_free(void *ptr)
{
    if (ptr) {
        free(ptr);
    }
}

/* this is almost same as safe free above but also sets the pointer to NULL */
void my_freep(void **ptr)
{
    if (*ptr != NULL) {
        free(*ptr);
    }

    *ptr = NULL;
}

and call my_freep like this my_freep((void**)&buffer);
NOTICE buffer here is defined as pointer (i.e. char *buffer;).
Hope that helps.

Answer 2

Allow me to compress your loop down to these key lines:

char *choice = (char*) malloc(sizeof(char));

do {

  scanf("%s", choice);
  switch (*choice) {
    case '6':
      printf("Exiting...\n");
      break;
    default:
      printf("Invalid choice\n");
  }

  if (choice != NULL)
  {
    free(choice);
  }

} while (*choice != '6');

This loop is seriously flawed. Not only are you using 1 byte of memory to store 2 (scanf should be using %c rather than %s), but if you pick anything other than 6, the loop will not only access freed memory, but it will also result in a double-free (your error). On the other hand, if you select 6 you are accessing memory after you have freed it, which is also a very serious error.

You should free the memory after the loop terminates.

Answer 3

Server side,

char *menu_choice = (char*) malloc(sizeof(char));

you allocate one char.

Then in get_menu_choice() called get_menu_choice(connfd, menu_choice) using the 1 byte allocated menu_choice,

void get_menu_choice(int socketNumber, char *choice)
{
   size_t n;

   readn(socketNumber, (unsigned char *) &n, sizeof(size_t));  
   readn(socketNumber, (unsigned char *) choice, n);

you store n bytes into choice...

That doesn't seem correct.

Is one byte allocation enough? Meaning, is n taking values > 1?

Answer 4

In a do loop you repeatedly free the pointer

if(menu_choice != NULL)
{
    free(menu_choice);
}

however you allocate it only once before the loop.