2

I'm a newbie to C, and I have been testing my program out in Fedora, using gcc and gdb to debug. I have a program that takes input from the user. If the first string entered is "create" then I take a look at the second command, and if that's "object" then I proceed to the createObject function.

Hopefully my code will make this a bit clearer:

static void parseCmd(char **input) {
    if(!strcmp(input[0], "create")) {
        if(!strcmp(input[1], "object")) {
            if(input[2] && strcmp(input[2], ""))
                createObject(input[2]);
            else
                printf("Object needs a name\n");
        }
        else
            printf("Command needs more parameters\n");
    }
    else
        printf("Command not recognized\n");
}

When I test entering just "create object" (no space after object, just the ENTER key)

In Linux it prints "Object needs a name"

But in windows the program crashes, it just hangs. How could I change the code to make it behave the same way as it does in Linux?

Doug Molineux
  • 12,283
  • 25
  • 92
  • 144
  • 2
    Your error is probably not in this section of the code. Could you post a short, complete, compilable example that demonstrates the problem? – Greg Hewgill Aug 03 '12 at 21:52
  • Writing C code that works in both Linux and Windows...wish I knew how to make all my C code do that 100% of the time too >. – Dennis Meng Aug 03 '12 at 21:53
  • 1
    `if(input[2] && strcmp(input[2], ""))` seems to evaluate to false in Linux when input equals "create object" but not in Windows, I'm not sure what it evaluates to in Windows actually – Doug Molineux Aug 03 '12 at 21:53
  • We really need to see what you're passing *in* to parseCmd. – paulsm4 Aug 03 '12 at 21:55

4 Answers4

3

You have code that says:

if(input[2] && strcmp(input[2], ""))

Apparently trying to detect if input[2] is present and not empty.

However, this will not work. There is NO promise that the lack of a third parameter will make input[2] be NULL or "".

input[2] will likely have a garbage value at that point, but garbage will still pass your test!

You have a few options to fix it.
Either, the caller of this function needs to guarantee that elements not set to valid data will instead be set to Zero/nil.

Or, the method I'd prefer, you change the function signature to something like this:

static void parseCmd(char **input, int num_inputs);

And pass the number of inputs along with the input-array.

abelenky
  • 63,815
  • 23
  • 109
  • 159
  • Great point about the num_inputs suggestion, I'll do this, I'm afraid I'll mark Corbin's as the answer though, since he was first, but both were correct, thank you for the response – Doug Molineux Aug 03 '12 at 22:04
2

You have a problem in this line :

if(input[2] && strcmp(input[2], ""))

input[2] in you test does not exist "create object", that's explain why it crash.

Hope this help.

Regards.

TOC
  • 4,326
  • 18
  • 21
1

You've entered into the realm of potential access violations or undefined behavior (well, undefined values):

input[2] && strcmp(input[2], "")

If the length of input is only 2, then this is reading past the acceptable point to read. Worst case this will cause a segfault (as Windows does), and best case the OS will let it happen and you'll get a random value in it. (Linux appears to be treating either it or *input[2] as 0 though.)

Anyway, rather than reading contents you are not allowed to access, pass in the length of the input and check that instead.

static void parseCmd( char **input, size_t num) { //or just int if size_t isn't already defined
    //compare num
}

--Edit--

As pointed out by Daniel Fischer, apparently argv[argc] is indeed a valid read, and it is guaranteed to be a null pointer.

Assuming that you are indeed passing argv to the function, this means that you could rely on this behavior. Your other two if statements do not check for this though, and for the sake of generalizing the function, it's still better to pass along the length (or, as paulsm4 said, you should look into the getopt function -- it makes parses parameters much easier than rolling your own parsing method).

Corbin
  • 33,060
  • 6
  • 68
  • 78
  • 1
    that's merely a guess: the problem could be something else entirely. As Greg Hewgill noted, a complete example would be helpful. Knowing how "input[]" is defined, initialized, and passed in is essential. IMHO... – paulsm4 Aug 03 '12 at 21:57
  • 1
    @paulsm4 You're correct, however, I believe you're adding unnecessary pickiness to the situation. Based on the context, there's an extremely high probably that `input` is just `argv`, and based on his statement that he's passing `create object`, argv will be of length 2, not 3. Between the code and that statement, I thought it was reasonable to make a conclusion that `input` is argv. When he responds to the comments, I will edit my answer if necessary. (And even if it's not argv, there should likely still be a length parameter for the sake of not overflowing.) – Corbin Aug 03 '12 at 22:00
  • Yep great point about the count of the individual strings in the actual input, I'm going to alter the code to pass this in, thanks Corbin – Doug Molineux Aug 03 '12 at 22:02
  • @Corbin: your mind-reading skills are better than mine - glad you were able to help him ;) Pete Herbert Penito: if you're going to pass argc/argv ... then why not also use the argument names argc/argv? Better, familiarize yourself with ["getopt()"](http://linux.die.net/man/3/getopt): http://www.ibm.com/developerworks/aix/library/au-unix-getopt.html IMHO... – paulsm4 Aug 03 '12 at 22:16
  • @Corbin if `input` is `argv` as in the argument strings passed to `main` (minus `argv[0]`), then that ought to be terminated with a `NULL`, 5.1.2.2.1 (2): "argv[argc] shall be a null pointer." Of course it will crash horribly if `input[0]` or `input[1]` is already `NULL`, so passing the length is the thing to do nevertheless. – Daniel Fischer Aug 03 '12 at 22:35
  • @DanielFischer Must admit that I never knew that. I'd always assumed that argv[argc-1] was the last valid access. Will make a note of that in the answer. – Corbin Aug 03 '12 at 23:12
1

You sir are hitting memory that you do not have access to be hitting.

I refer you to

How to check if a pointer is valid?

Anyone who wants to know how he is passing things in:

int main(void) 
{ 
    char* values[] = {"1", "2", "3"};
    parseCmd(values);
}

It is happening when you try to access input[2] but it could happen at any if you do not know what you are taking in. I would reccomend sanity checking when you are getting the input. If they dont put anything in values[2] make your function aware of that or change the way it handles it. With pointers and pointers to pointers you can't skimp on the sanity checking when you assign the values.

Community
  • 1
  • 1
Steve
  • 652
  • 3
  • 11
  • 23