strcpy resulted in bad access or fault segmentation

Question

Here is a shell program I wrote for honing my understanding on C pointer and array. This shell program has the functionalities of reading in commands (execute them in execvp()) and the history feature can store 10 commands (similar to Linux terminal) where "!!" returns latest command and execute, !Nth returns Nth command and execute, otherwise read command and store them in history array. When requested history command doesn't exist, relevant error message printed. For full code reference, see Shell_Program_C.

initialize() method initialize each history character string array with letter 'e' ('e' means empty or string hasn't been assigned) so that I could later check that history string has value or not. readHist() method assign each individual command to the history string array, char** hist which has a size of 10.

My problem is:strcpy(hist[histC], tokens[count])in readHist() is returning 'bad access' here.

Expected behavior: each command should be copied to char** hist as a string of char so that later requested command can be retrieved and executed.

char** initialize() {
char** hist = malloc(10 * sizeof(char *));
for (int i = 0; i < 10; ++i) {
        hist[i] = (char *)malloc((MAX_LINE / 2) + 1);
    for(int j = 0; j < (MAX_LINE / 2); j++) {
        hist[i][j] = 'e';
    }
}
return hist;
}


void readHist(char**hist, int histC ,char** tokens, int count) {
histC = (histC - 1) % 10;
for(int i = 0; i < count; i++) {
    size_t strlenth = strlen(tokens[i]);
    //strcat(hist[histC], tokens[count]);
    if(count > 1) {
       strcat(hist[histC], tokens[count]);
       hist[histC][strlenth] = ' ';
    } else {
        printf("histC%d", histC);
       strcpy(hist[histC], tokens[count]); // bad access or segmentation fault
       hist[histC][strlenth] = '\0';
    }
}
}

Answer 1

Your SegFault is due to violating the strcat requirement that "The strings may not overlap" This looks like a careless oversight that has big consequences. Specifically, look at cmdargv below:

histCheck(cmdargv, &histCount, cmdargv, count, &isAmp);

You are passing the same array of pointers cmdargv as the parameters that are passed to hist and tokens in your call to:

void histCheck(char**hist, int* histCount, char**tokens, int tcount, int* amp)

In histCheck both hist and tokens point to the same memory that can be seen in a debugger, e.g.

histCheck (hist=0x7fffffffd9b0, histCount=0x7fffffffd9a8, tokens=0x7fffffffd9b0

note the address of both hist and tokens.

This same error is then passed to readHist, e.g.

readHist (hist=0x7fffffffd9b0, histC=1, tokens=0x7fffffffd9b0

which results in your call to strcat attempting to concatenate overlapping strings (the same string), e.g.:

strcat (hist[histC], tokens[count]);

(note: are you sure you want count above?)

which results in your familiar:

Program received signal SIGSEGV, Segmentation fault.
__strcat_sse2_unaligned () at ../sysdeps/x86_64/multiarch/strcpy-sse2-unaligned.S:296
296     ../sysdeps/x86_64/multiarch/strcpy-sse2-unaligned.S: No such file or directory.

Now I'm not saying your problems are limited to that (note my comment about he update to count in checkAmp), so you have more work to do. But your SegFault is no longer a mystery.

It appears you intended:

histCheck (hist, &histCount, cmdargv, count, &isAmp);

You will also want to revisit:

strcat (hist[histC], tokens[count]);

which will lead to your very next SegFault. Here it appears you want:

strcat (hist[histC], tokens[i]);

Also, in your rework of your code, pay careful attention to resetting all values with each iteration around your do loop in main to prevent leaving stray values in cmdargv, etc... You will also need some way to free the memory you allocate to prevent leaking memory on subsequent calls to malloc in your various functions.

You will want to make friends with a debugger. There is no way around it, especially when you are chopping the flow up into a number of small inter-related functions to compartmentalize the code. Nothing wrong with that, it just make the debugger all the more important to figure out where the wheels are coming off -- and why...

Rework your code and post any further problems you have. To bandaid the count problem in checkAmp, you may consider something like:

void checkAmp (int *count, char** tokens, int *Amp) {
        size_t strlenth = strlen(tokens[*count - 1]);
    if((strlenth == 1) && tokens[*count - 1][0] == '&') 
    {    
        *Amp = 1;     
        tokens[*count - 1] = NULL; 
        *count = *count -1;
    } else {
        tokens[*count] = NULL;
    } 
}

(and updating the call to checkAmp as well)

Lastly, while not an error, the standard coding style for C avoids the use of caMelCase or MixedCase variable names in favor of all lower-case while reserving upper-case names for use with macros and constants. It is a matter of style -- so it is completely up to you...