using strcpy for copying string into the element at index retrieved with atoi

Question

Here's the code, which is supposed to execute the first command in history when "history 1" is entered:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main (int argc, char *argv[])
{
    int i=0; int j=0; int k=0;
    char inputString[100];
    char *result=NULL;
    char delims[] = " ";
    char historyArray[100][100] = {0};
    char *tokenArray[100][100] ;

    do
    {
        j = 0;
        printf("hshell>");
        gets(inputString);
        strcpy (historyArray[k], inputString);
        k++;

        // Break the string into parts
        result = strtok(inputString, delims);

        while (result!=NULL)
        {
            //result2 = result;
            strcpy(tokenArray[j], result);
            //puts(result);
            j++;
            result= strtok(NULL, delims);                  
            //puts(tokenArray[j]);     
        }
        //j = 0;
        puts(tokenArray[0]);
        puts(tokenArray[1]);

        if (strcmp(tokenArray[0], "exit") == 0)
        {
            return 0;
        }
        else if (strcmp(tokenArray[0], "history") ==  0)
        {
           if (j>1)
           {
              strcpy (result,historyArray[atoi(tokenArray[j-1])]);

           }
           else
           {
               //print history array
               for (i=0; i<k;i++)
                   printf("%i. %s\n", i+1, historyArray[i]);
           }
        }
        else
        {
          printf("Command not found\n");
        }
    }while (1);
}

However, it crashes. When in debugging, I noticed two things: - the array (tokenArray) address is out of bounds and - Access Violation (Segmentation Fault). You can see the errors in the images below.

Out of bounds

Segmentation Fault

What am I missing? What am I doing wrong?

I think it should be `char tokenArray[100][100]`, i.e. without the `*`. — cnicutar, Feb 24 '13 at 12:29
Also note that `historyArray[atoi(tokenArray[j-1])]` is quite risky... and also `atoi` is not part of standard C, use `sscanf(tokenArray[j-1], "%d", &index)` instead and check whether its return value is `1` (meaning that 1 integer has really been read), then just access `historyArray[index]` :) — LihO, Feb 24 '13 at 12:30
Where should I use `sscanf(tokenArray[j-1], "%d", &index)` exactly? Im still new in this language. Can you give me an example? — serge, Feb 24 '13 at 12:34
@cnicutar tried that. The warnings dissappear, but it still crashes. — serge, Feb 24 '13 at 12:35
@cnicutar: Ah, yeah. Sorry :) But I would use `sscanf` because of easier error identification anyway. — LihO, Feb 24 '13 at 12:40
@bmargulies I need to learn alot of things obviously. Im working on it. — serge, Feb 24 '13 at 12:42
but specifically, people here are unlikely to debug your code by reading it. Use the debugger to see what's going wrong. Set breakpoints. trace execution. — bmargulies, Feb 24 '13 at 12:42
Don't use `gets()`; use `fgets()` instead. It probably isn't the cause of your current trouble, but it is a major liability down the road. Forget that `gets()` ever existed; it isn't in C2011 (but was in C89 and C99). Never use it. — Jonathan Leffler, Feb 24 '13 at 15:07

LihO · Accepted Answer · 2013-02-24T14:13:35.723

1

The reason why you are dealing with segmentation fault is because you are trying to copy a string into the memory that has not yet been allocated. You have defined result as a char* and just assigned NULL to it, so trying to copy string into it is wrong:

char *result = NULL;
// ...
strcpy(result, historyArray[atoi(tokenArray[j-1])]);

You need to allocate some memory, that result will point to. Then strcpy can be used to copy string into this memory. You can either use malloc to allocate it dynamically or you can define result as an temporary variable with automatic storage duration (i.e. char result[100];).

Also note that

char *tokenArray[100][100];

defines a two-dimensional array of pointers to char. But what you actually need in this case is an array of strings, so you need to get rid of * just like @cnicutar has pointed out.

And one more note:

strcpy(result,historyArray[atoi(tokenArray[j-1])]);

is quite dangerous thing to do, because when atoi fails, you are trying to access the element out of array bounds, which produces undefined behavior, thus I recommend you doing something like this:

char tokenArray[100][100] = {0};

int index;
char indexString[100] = "8";
if (sscanf(indexString, "%d", &index) == 1)     // integer successfully retrieved
{
    strcpy(tokenArray[index], "some string");
    printf("%s", tokenArray[8]);
}

edited Feb 24 '13 at 14:13

answered Feb 24 '13 at 13:36

LihO

41,190
11
99
167

I used this `strcpy(hCommand,historyArray[tempIndex-1]);` to try to copy the first command in `historyArray` in the `hCommand variable. I declared the `hCommand` like this: `char hCommand[1][20];`. It produced `array subscript is not an integer` . Any ideas why? – serge Feb 24 '13 at 17:25
@voth1234: It means that `tempIndex` should be of type `int`. – LihO Feb 24 '13 at 17:26
missed that :) thank you. EDIT: did that, compiled. Now it crashed `(Segmentation Fault)` as the previous line `strcpy (tempIndex,tokenArray[1]);` – serge Feb 24 '13 at 17:28
@voth1234: You should learn how to work with debugger. Also make sure you always understand what is your code doing, especially when it goes to memory management. You should know when and what is being allocated, how to work with it, whether you are the one responsible for freeing it and if yes then how... if you are stuck with something that you can't solve on your own, then ask it as a new question. Just make sure you put some effort of your own into it first :) – LihO Feb 24 '13 at 18:13

score 1 · Answer 2 · answered Feb 24 '13 at 14:42

You probably meant char tokenArray[100][100]; which creates 100 tokens with 100 characters each in 1 token.

writing char *tokenArray[100][100] literally means tokenArray is an array of 100 arrays, which contain 100 char *. But each of those char * points to a random addresses if it is not assigned a proper address.

You are getting a segmentation violation error because one of the char * contains an address which you cannot access.

using strcpy for copying string into the element at index retrieved with atoi

2 Answers2