2

Each line of input is a line with a command followed by numbers (except in the exit case).

I cannot figure out what I am doing wrong. This segment is looking for the store command and then does the action store entails:

    char command[20];
    while(strcmp(command, "exit") != 0)
    {
        /*scans for command strings inputted*/
        scanf(" %s", command);
        /* handles store command*/
        if(strcmp(command, "store") == 0)
        {   
            memory[0] = 1;
            scanf("%d %d %d %d %d", &startx, &starty, &finishx, &finishy, &number);
            for( i = startx; i < finishx; i++)
            {
                for(j = starty; j < finishy; j++)
                {
                square[i][j] = number;
                }
            }
        }
     }
Michael Kingsmill
  • 1,815
  • 3
  • 22
  • 31
  • 1
    What is the problem you are having? What have you tried to resolve the issue? – Michael Kingsmill Feb 27 '12 at 03:14
  • Its been a long time, but I avoided scanf like the plague. IIRC if you tried scanf %d and typed in something other than an integer the program would crash. IIRC I mostly used `fgets()` to read a line of input, validate it and then convert it as necessary. – BillRobertson42 Feb 27 '12 at 03:24

1 Answers1

9

Yes, you are using it wrongly(a). The line:

scanf(" %s", command);

has no bounds checking on the input. If someone enters more than 19 characters into your program, it will overflow the char command[20] and result in undefined behaviour.

The two major problems with scanf are:

  • using it with an unbounded %s since there's no way to control how much data is entered. My favourite saying is that scanf is to scan formatted information and there's not much less formatted than user input.
  • not checking how many items were scanned - it's possible to scan less than you expected.

If you want to do it right, see here. It uses fgets to get a line, protecting from buffer overflows and detecting problems.

Once you have the line as a string, you can safely sscanf it to your heart's content, since you know the upper bound on the length and you can always return to the start of the string to re-scan (not something easy to do with an input stream).


Other problems with your code include:

  • The initial strcmp on an uninitialised buffer. It could actually be (arbitrarily) set to exit which would cause your loop not too start. More likely is that it's not a valid C string at all, meaning strcmp will run off the end of the buffer. That won't necessarily end well.
  • No checking that all your numeric items were entered correctly. You do that by checking the return value from scanf (or sscanf should you follow my advice on using the rock-solid input function linked to here) - it should be five but entering 1 hello 2 3 4 would result in a return code of one (and all but startx unchanged).
  • No range checking on input. Since square has limited dimensions, you should check the inputs to ensure you don't write outside of the array. That's both numbers that are too large and negative numbers.

(a) Syntactically, what you have is correct. However, from a actual semantic point of view (i.e., what you mean to happen vs. what may happen), there's a hole in your code large enough to fly an Airbus A380 through :-)

paxdiablo
  • 854,327
  • 234
  • 1,573
  • 1,953
  • This was a very nice and informative response. I have voted to delete my response as I acknowledge yours was more useful. – Stephen Tetreault Feb 27 '12 at 03:35
  • Note that the correct format specifier is `"%19s"`; using `"%20s"` would be a mistake leading to overflow - and, since `command` is a local variable, it would be a stack overflow. – Jonathan Leffler Feb 27 '12 at 03:46
  • 2
    +1 For a nice answer: properly diagnosing the issue, describing the solution, and providing a link to good "model" are the trifecta. I wish all SO answers were like this. – Peter Feb 27 '12 at 03:52