C function for copying array into matrix

Question

I want a matrix that can grow dynamically and build up a structure in RAM as input arrives from standard input och terminal (shell). First I tried with a matrix like A[nrows][ncols] but I changed to the char **matrix[32]; and I want to refactor so that there are fewer constants and that malloc will be safe.

char ***write_command(int row, int argc, char *argv[], char **string[]) {
    assert(argv);
    assert(row > -1);
    assert(argc > -0);
    int len = 0;
    for (int j = 0; j < argc; j++) {
        if (argv[j]) {
            len = len + (int) strlen(argv[j]);
        }
    }
    string[row] = malloc(row * argc * sizeof(char));
    for (int j = 0; j < argc; j++) {
        if (argv[j]) {
            string[row][j] = strdup(argv[j]);
        }
    }
    return string;
}

I use the above function in a loop like this

matrix = write_command(row++, *argc, *argv, matrix);

It doesn't crash and has correct output but I suspect wrongdroings with malloc because there is a log about this from Valgrind saying something about the function write_command.

==29296== Invalid write of size 8
==29296==    at 0x4030EE: write_command (main.c:129)
==29296==    by 0x4030EE: runCmd (main.c:405)
==29296==    by 0x4030EE: command (main.c:704)
==29296==    by 0x40194E: main (main.c:803)
==29296==  Address 0x5899c90 is 0 bytes after a block of size 0 alloc'd
==29296==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29296==    by 0x4030C0: write_command (main.c:126)
==29296==    by 0x4030C0: runCmd (main.c:405)
==29296==    by 0x4030C0: command (main.c:704)
==29296==    by 0x40194E: main (main.c:803)
==29296== 
 {ls} {|}
==29298== Invalid read of size 8
==29298==    at 0x405903: fork_pipes (in /home/dac/ClionProjects/shell2/openshell/shell)
==29298==    by 0x4036AC: runCmd (main.c:442)
==29298==    by 0x4036AC: command (main.c:704)
==29298==    by 0x40194E: main (main.c:803)
==29298==  Address 0x5899c90 is 0 bytes after a block of size 0 alloc'd
==29298==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29298==    by 0x4030C0: write_command (main.c:126)
==29298==    by 0x4030C0: runCmd (main.c:405)
==29298==    by 0x4030C0: command (main.c:704)
==29298==    by 0x40194E: main (main.c:803)
==29298== 
==29298== Syscall param execve(argv) points to unaddressable byte(s)
==29298==    at 0x513DCF7: execve (syscall-template.S:84)
==29298==    by 0x513E50A: execvpe (execvpe.c:146)
==29298==    by 0x405910: fork_pipes (in /home/dac/ClionProjects/shell2/openshell/shell)
==29298==    by 0x4036AC: runCmd (main.c:442)
==29298==    by 0x4036AC: command (main.c:704)
==29298==    by 0x40194E: main (main.c:803)
==29298==  Address 0x5899c90 is 0 bytes after a block of size 0 alloc'd
==29298==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29298==    by 0x4030C0: write_command (main.c:126)
==29298==    by 0x4030C0: runCmd (main.c:405)
==29298==    by 0x4030C0: command (main.c:704)
==29298==    by 0x40194E: main (main.c:803)
==29298== 

The test with Valgrind is scripted to run when I make changes to the code is scripted and makes command-line input to the write_command which writes a command to the command matrix that is then executed by fork_pipes and exec.

## RUN_TESTS ##

#!/bin/sh
echo "-- Testing our implementation of OpenShell --"
echo ""
echo "- If you have any problem in passing a test read the corresponding"
echo "- source file to understand what the test is checking"
echo ""
printf "********************* PRESS ENTER TO RUN TESTS  ... "
#read _
make
valgrind --leak-check=yes ./shell .<< EOF
ls -al|grep open|awk '{print \$9}'
EOF
printf "********************* TEST WILDCARDS \n***** Press any key to listing all files in current directory...\nYou should see filesnames *.* below "
read _
./shell << EOF
ls
EOF
#printf "********************* TEST ALGORITHMS ...  \n***** Press any key to run the algorithms... .\nYou should see the output from top -b -n1|head -8|tail -1 "
#read _
#valgrind./shell << EOF
#top|head -8|tail -1|sort -n|wc -l
#EOF

#printf "********************* TEST ALGORITHMS Part II.  ... .\nYou should see the output from who|awk '{print \$4 ; print \$3}'|sort -n|wc -l. "
#read _
#valgrind ./shell << EOF
#who|awk '{print \$4 ; print \$3}'|sort -n|wc -l
#EOF

#printf "********************* TEST CHECKENV.  ..... .\nYou should see the output checkenv below "
#read _
#valgrind ./shell << EOF
#checkenv
#EOF
#printf "********************* TEST DONE. YOU SHOULD SEE OUTPUT FROM TEST ABOVE ... "
#read _

The struct I use that is the "leaf" of the structure is

struct command {
    char *const *argv;
};

Then I can build up commands and pipelines with fork and exec.

/* Helper function that forks pipes */
void fork_pipes(int n, struct command *cmd) {
    int i;
    int in = 0;
    int fd[2];

    for (i = 0; i < n - 1; ++i) {

        if (pipe(fd) == -1) {
            err_syserr("Failed creating pipe");
        }

        spawn_proc(in, fd[1], cmd + i);
        close(fd[1]);
        in = fd[0];
    }
    if (dup2(in, 0) < 0) {
        err_syserr("dup2() failed on stdin for %s: ", cmd[i].argv[0]);
    }
    /*fprintf(stderr, "%d: executing %s\n", (int) getpid(), cmd[i].argv[0]);*/
    fprintf(stderr, "\n");
    execvp(cmd[i].argv[0], cmd[i].argv);
    err_syserr("failed to execute %s: ", cmd[i].argv[0]);
}

/* Helper function that spawns processes */
int spawn_proc(int in, int out, struct command *cmd) {
    pid_t pid;
    fflush(NULL);
    pid = fork();
    if (pid == 0) {
        if (in != 0) {
            if (dup2(in, 0) < 0)
                err_syserr("dup2() failed on stdin for %s: ", cmd->argv[0]);
            close(in);
        }
        if (out != 1) {
            if (dup2(out, 1) < 0)
                err_syserr("dup2() failed on stdout for %s: ", cmd->argv[0]);
            close(out);
        }
        /*fprintf(stderr, "%d: executing %s\n", (int) getpid(), cmd->argv[0]);*/
        fprintf(stderr, "[%d]\n", (int) getpid());
        execvp(cmd->argv[0], cmd->argv);
        err_syserr("failed to execute %s: ", cmd->argv[0]);
    }
    else if (pid < 0) {
        err_syserr("fork failed: ");
    } else {
        /* printf("** we are the parent ***"); */
    }
    return pid;
}

I call the above functions by creating a structure from the matrix.

for (int i = 0; i < n; i++) {
    shellcommand[i].argv = matrix[i];
}

Answer 1

string[row] = malloc(row * argc * sizeof(char));

This line isn't making any sense. Why would you factor the row index in? And why sizeof(char)?

Shouldn't this be the following instead?

string[row] = malloc(argc * sizeof(char*));

That would explain why valgrind complains, you didn't allocate enough memory to hold the pointers.

---

if (argv[j]) {
  string[row][j] = strdup(argv[j]);
}

This only protects you from accessing a null pointer in argv[j], but in return it now leaves string[row][j] uninitialized. Not "set to 0s", but actually uninitialized so it contains whatever garbage was previously at the memory address.

---

This fragment is entirely dead code, btw:

int len = 0;
for (int j = 0; j < argc; j++) {
    if (argv[j]) {
        len = len + (int) strlen(argv[j]);
    }
}