Homework: I have a memory leak somewhere, but I can't find it. Any tips on how to use valgrind more effectively?

Question

Okay, first off, fair warning, this is a project for a class. I am not looking for any help on it other than fixing my memory leak. I imagine I have followed some horrendous coding practices over the space of this C code. Regardless, when I run Valgrind to search for where the memory leaks are happening, it is not at all clear to me what memory leaks I am missing. I know that there are at least two strings that I am not freeing, but that I malloc()ed, based purely on the size of the valgrind output. Because I took some of the extraneous code out of the project, the valgrind line numbers, may very well be off, because of this I marked them with comments for your convenience.

A little background, I am writing the basic functions of a shell. It currently does the following:

1.Take user input

2.Parse the input into a cmdin struct

3.Execute the command, provided it doesn't have a pipe in it.

4.free the space from the cmdin I created, and start over at step 1. This is where the problem occurs.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>
#define MAX_SIZE 1024
#define MAX_CLEN 2097152
#define READ_END 0
#define WRITE_END 1
#define CHILD_STATUS 0
struct cmdin
{
    char *cmd;
    char **args;
    int nargs;
    int pipeflag;
};

//check if the last argument of the command passed to it is a pipe.
//if so, return the position of the pipe, otherwise, return 0
//if it has already found a pipe, it will return the position of that pipe
int conpipe(struct cmdin * cmd)
{
    if(!cmd->pipeflag)
    {
        if(!strcmp(cmd->args[cmd->nargs], "|"))
        {
            return cmd->nargs;
        }
        else
        {
            return 0;
            /* PROBLEM LINE BELOW */
        }  // line 46, where valgrind claims one of the problems exists
    }
    else
    {
        //printf("pipeflag: %d\n", cmd->pipeflag);
        return (cmd->pipeflag);
    }
}

//free the command after each runthrough
int freeze(struct cmdin cmd)
{
    int i;
    for(i=0; i <= (cmd.nargs); i++)
    {
        //printf("cmd.args[%d]: %s\n",i, cmd.args[i]);
        /* PROBLEM LINE BELOW*/
        free(cmd.args[i]); //this is line 62, as noted by valgrind
    }
    free(cmd.args);
    free(cmd.cmd);
    return 0;
}

//parse input, and add a null to the end
struct cmdin * parse(char *cmd)
{
    //allocate space for the command 
    struct cmdin *ped = malloc(sizeof(struct cmdin)); 
    //declare pipeflag, and nargs as 0
    ped->pipeflag = 0;
    ped->nargs = 0;
    //allocate space for the array of strings, and for the string cmd.
    ped->args =  malloc(sizeof(char*) * MAX_SIZE);
    ped->cmd = malloc(sizeof(char) * MAX_SIZE);
    //scan the input, and put the first argument into the cmd string
    sscanf(cmd, "%s %[^\n]", ped->cmd, cmd);
    //allocate space for the next string, and then copy cmd to it.
    ped->args[ped->nargs] =  malloc(sizeof(char) * MAX_SIZE);
    strcpy(ped->args[ped->nargs], ped->cmd);
    ped->pipeflag = conpipe(ped);

    /* PROBLEM LINE BELOW*/
    ped->nargs++;     // line 86, where valgrind claims the second leak is called?
    ped->args[ped->nargs] =  malloc(sizeof(char) * MAX_SIZE);
    //loop that allocates space for a command, and then assigns
    //the next arg to it.
    while(sscanf(cmd, " %s %[^\n]", ped->args[ped->nargs], cmd) == 2)
    {
        ped->pipeflag = conpipe(ped);
        ped->nargs++;
        ped->args[ped->nargs] = malloc(sizeof(char) * MAX_SIZE);
    }

    strncpy(ped->args[ped->nargs], cmd, strlen(cmd)-1);
    ped->nargs++;
    //ped->args[ped->nargs] = malloc(sizeof(char) * MAX_SIZE);
    ped->args[ped->nargs] = NULL;

    return ped;

}

int main()
{
    char cwd[MAX_CLEN];
    getcwd(cwd, sizeof(cwd));
    char input[MAX_CLEN]; 
    char *exit  = "exit\n";
    char *cd = "cd";

    //Command to take input
    printf("tosh$ ");
    fgets(input, sizeof input, stdin);    

    while(strcmp(input, exit))
    {
       //Command to parse input
       struct cmdin *cmd;
       cmd = parse(input);
       //if there is not a pipeflag
       if(!cmd->pipeflag)
       {
            //Change directories
            if(!strcmp(cd, cmd->args[0]))
            {
                chdir(cmd->args[1]); 
                getcwd(cwd, sizeof(cwd));
            }
            else if(strcmp(input, exit))
            {
                //command to run input
                int child_pid;
                child_pid = fork();
                if(child_pid == 0)
                {
                    if(strcmp(cmd->args[1],cmd->args[0]))
                    {
                        execvp(cmd->cmd, cmd->args);
                    }
                    else
                    {
                        free(cmd->args[1]);
                        cmd->args[1] = NULL;
                        cmd->nargs--;
                        execvp(cmd->cmd, cmd->args);

                    }
                }
                else
                {
                    wait(&child_pid);
                }
            }
            freeze(*cmd);
            free(cmd);
        }       
        //Command to take input
        printf("tosh$ ");
        fgets(input, sizeof input, stdin);
    }
    return 0;
}

Note: the input for this valgrind, looked like this:

tosh$ ls -al
tosh$ exit

The valgrind Heap and Leak Summaries, are as follows:

HEAP SUMMARY:
==4901==     in use at exit: 4,096 bytes in 2 blocks
==4901==   total heap usage: 6 allocs, 4 frees, 24,600 bytes allocated
==4901== 
==4901== 2,048 bytes in 1 blocks are definitely lost in loss record 1 of 2 
==4901==    at 0x4C2B3F8: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4901==    by 0x400A48: parse (tosh.c:46)
==4901==    by 0x400C97: main (tosh.c:86)
==4901== 
==4901== 2,048 bytes in 1 blocks are definitely lost in loss record 2 of 2
==4901==    at 0x4C2B3F8: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4901==    by 0x400BDE: parse (tosh.c:62)
==4901==    by 0x400C97: main (tosh.c:86)
==4901== 
==4901== LEAK SUMMARY:
==4901==    definitely lost: 4,096 bytes in 2 blocks
==4901==    indirectly lost: 0 bytes in 0 blocks
==4901==      possibly lost: 0 bytes in 0 blocks
==4901==    still reachable: 0 bytes in 0 blocks
==4901==         suppressed: 0 bytes in 0 blocks

UPDATE: As per request. My Makefile:

CC=gcc
CFLAGS=-g -Wall
TARGET=tosh

$(TARGET): $(TARGET).c
    $(CC) $(CFLAGS) -o $(TARGET) $(TARGET).c

Valgrind Version: valgrind-3.7.0

gcc Version: gcc (Ubuntu/Linaro 4.7.2-2ubuntu1) 4.7.2

echo $LD_PRELOAD:

(This printed nothing)

Answer 1

Have not managed to reproduce. But a couple of notes.

1. Use what you have available of error and warnings.

   gcc -Wall -Wextra -pedantic …
   

2. Check return values and success of operations. This is imperative for reducing bugs, especially those that only a user case a mile down the road reveals.

if (!(foo = malloc(size))) {
    perror("mem");
}

...


/* This line is no good: */
sscanf(cmd, "%s %[^\n]", ped->cmd, cmd);
/*      |                           |
 *      +----------+----------------+
 *                 |
 *                 +--- Not good. UB. (Input / Output same.)
 */

You could add a routine to get count and sizes ...

i = sscanf(cmd, "%s%n %[^\n]%n", ped->cmd, &n1, cmd, &n2);

if (i < 2) {
    /* Debug print. */
    fprintf(stderr, 
            "%s:%-3d; "
            "sscanf => %d items and %d bytes str[%d]<%s>\n", 
            __FILE__, __LINENO_,
            i, n2, n1, ped->cmd
    );
}

etc.