0
\$\begingroup\$

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];
}
\$\endgroup\$
2
\$\begingroup\$
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]);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Yes, I think it perhaps should be string[row] = malloc(strlen(cmd) * sizeof(char*));where cmd is a char * with the current pipeline e.g for a pipeline such as ls -al|grep code then cmd will be 6 the first iteration and 9 the next iteration (grep code\0) which is what I'm getting but I'm also getting leaks with Valgrind (though the program runs "correct" with the expected output). I'm going to have to learn malloc more. \$\endgroup\$ – Niklas May 4 '16 at 21:37
  • 1
    \$\begingroup\$ No, you are just mixing up random datatypes now. Your matrix holds pointers on pointers, no plain char array. So all you are going to malloc is space to store pointers, and you just need to know how many pointers. strdup() is responsible malloc'ing the memory to hold the actual strings, and you are reseving the memory for the pointer strdup() returns. \$\endgroup\$ – Ext3h May 4 '16 at 21:41
  • 1
    \$\begingroup\$ Why exactly did you need if (argv[j])? There should never, never be nullpointers in there. If there are, you have already broken something much sooner. Try debugging this properly with gdb or any graphical frontend please. Even worse - if there were, these null-pointers are now replaced by random garbage left over by malloc. (malloc does not initialize the memory to 0!) \$\endgroup\$ – Ext3h May 4 '16 at 22:05
  • 1
    \$\begingroup\$ With argc * sizeof(char*) that should not happen. Can you please use a debugger to see in which loop iteration this problem occurs? 8 bytes is just one pointer too much, but I don't see an off-by-one error in the code. \$\endgroup\$ – Ext3h May 4 '16 at 23:30
  • 1
    \$\begingroup\$ I fíxed the error, it was a memory alignment error because of my allocation. I do it this way now: char *** matrix = alloc_matrix(BUFFER_SIZE, BUFFER_SIZE);´ and then a function for initalizing it where allocation is done char*** matrix = malloc(rows * sizeof(char**)); and a function for freeing it. It was a data alignment error because I'm not very experience with pointer arithmetic and detailed C but I'm learning and it's fun. Notice that the mistake I made was to initilize a row and then empty rows in the row instead of ROW+1 iniitializations. \$\endgroup\$ – Niklas May 5 '16 at 6:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.