Why is one seg faulting and the other not?

Question

Hi I have a program that needs to compare a string array to a predefined string but when I use the variable args[0] it works in my function strcmp as below

int gash_execute(char **args){
    int i;

    if(args[0] == NULL){
        return 1;
    }

    for(i = 0; i < gash_command_num(); i++){
        if(strcmp(args[0], functions[i]) == 0){
            return(*function_address[i])(args);     
        } 
    }
    return gash_launch(args);
}

However when trying to strcmp args[i] such as below I get a seg fault. Can anyone help me find a solution to this problem?

int gash_execute(char **args){
    int i;

    if(args[0] == NULL){
        return 1;
    }

    for(i = 0; i < gash_command_num(); i++){
        if(strcmp(args[i], functions[i]) == 0){
            return(*function_address[i])(args);     
        } 
    }
    return gash_launch(args);
}

args[] is an array of strings that were previously separated by spaces, this program is for a custom shell, so pretend in my shell command line I input "cat echo ls" args[0] would be "cat" and so on. However now I need to implement i/o redirection. So i need to check every element of args to check if they represent the symbols "<" ">" "|" and if one of them is we can take it from there

You are reading too far into the args array. Since you don't explain what any of your variables represent, it's hard to say much more. — jforberg, Apr 19 '16 at 22:21
What are `gash_command_num`, `functions`, `function_address`, and `gash_launch`? Better said: where's your Minimum, Complete, and Verifiable Example? http://stackoverflow.com/help/mcve — inetknght, Apr 19 '16 at 22:21
@t0mm13b args[0] does not point to anything. You cannot tell what it points to from this example. — Harry, Apr 19 '16 at 22:22
@t0mm13b We definitely can't infer that from this example. Args could be anything, and "gash" is not a library that I can find published anywhere. — jforberg, Apr 19 '16 at 22:24
@t0mm13b You're assuming he's passing in argv from mains paramater list. You cannot infer that here. — Harry, Apr 19 '16 at 22:24
args[] is an array of strings that were previously separated by spaces, this program is for a custom shell, so pretend in my shell command line I input "cat echo ls" args[0] would be "cat" and so on. However now I need to implement i/o redirection. So i need to check every element of args to check if they represent the symbols "<" ">" "|" and if one of them is we can take it from there. — s.gang, Apr 19 '16 at 22:25
It would be clearer if you passed in the number of valid arguments (that is, the number of valid pointers in `args`) as a second argument to `gash_execute`. — Steve Summit, Apr 19 '16 at 22:34
Possible duplicate of [Definitive List of Common Reasons for Segmentation Faults](http://stackoverflow.com/questions/33047452/definitive-list-of-common-reasons-for-segmentation-faults) — CodeMouse92, Apr 19 '16 at 22:38

Schwern · Accepted Answer · 2016-04-19T23:53:10.247

Without seeing all the code, or a report from a tool such as valgrind, I can't say for sure. But I can tell you that this loop is full of potential problems.

for(i = 0; i < gash_command_num(); i++){
    if(strcmp(args[i], functions[i]) == 0){
        return(*function_address[i])(args);     
    } 
}

It's iterating through three arrays (args, functions, and function_address) based on some function call which takes none of those as variables (gash_command_num()) which has an unknown relation to how many elements are actually in those arrays.

And it's using two global variables (functions and function_addresses) which could contain anything.

If all those things are related, I would suggest making that explicit... but I suspect they're not. I suspect your loop logic is wrong. It's comparing args[i] with functions[i]. I suspect gash_command_num() is actually the size of functions and so the loop is walking off args.

What I suspect you really want to do is to see if args[0] matches any function name in functions and then call the related function. If args[0] is ls then you want to check if there's a built in shell function for ls and call it with all the arguments.

Rather than searching a list over and over again, and having to manage two parallel lists, this would be much better served with a hash table. The keys are the function names, the values are the function pointers. C doesn't have a hash table built in, but there's plenty of libraries for that. Gnome Lib is a solid choice for this and many other basic functionality that C is missing.

Using a hash table, and eliminating the globals, your code reduces to this:

/* For encapsulation */
typedef int(*shell_function_t)(char **);

int gash_execute(char **args, GHashTable *shell_functions){
    int i;

    if(args[0] == NULL){
        return 1;
    }

    shell_function_t func = g_hash_table_lookup(shell_functions, args[0]);
    if( func ) {
        return(*func)(args);
    }
    else {
        return gash_launch(args);
    }
}

Scanning for pipes is now a separate problem. For that you do want to loop through args looking for special characters. That is made much simpler if, when you create args, you ensure it ends with a null pointer.

for(int i = 0; args[i] != NULL; i++) {
    ...check if args[i] is special...
}

Why is one seg faulting and the other not?

1 Answers1