Why is this strchr program a segfault?

[HelloJarvis]

Hey guys,

I'm working with strchr and, for some reason, am getting a segfault. My program takes a string, iterates through the characters of that string, and adds a score to the "game" based on what letter the current character is. That sounds complicated, but my code will explain it:

// initialize alphabet and corresponding scores as arrays
char alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
int scores[] = {1, 4, 4, 2, 1, 4, 3, 3, 1, 10, 5, 2, 4, 2, 1, 4, 10, 1, 1, 1, 2, 5, 4, 8, 3, 10};

char word[] = "hello";

// iterate through word
for (int i = 0; i < strlen(word); i++)
{
    // get current character in alphabet, update 
    // score based on corresponding letter score
    char* c = strchr(alphabet, word[i]);

    while (c != NULL)
    {
        int index = c - word;
        score += scores[index];
    }
}

This gives a segfault. What am I doing wrong? Thanks in advance!

[HelloJarvis]

Congratulations! You're no longer a DaniWeb newbie.<br /> <br />
Your DaniWeb account has just been upgraded from newbie status and now you have the ability to take advantage of everything the community has to offer.<br /> <br />
You can now enjoy an advertisement-free DaniWeb by ticking the checkbox to Disable Ads in your profile. You will no longer have to fill out the human verification check when you post. You can also now send unlimited private messages, contribute new code snippets, and tag articles with never-before-used tags.

[dx9_programmer]

There are several problems with your code. A segmentation error most likely means you access a variable outside the range of an array of some sort, such as your scores or alphabet array.

1. The variable score (singular) is not declared.

2. The while loop should be an if statement instead.

3. I recommend when comparing characters you convert the input to all lowercase or uppercase. Because 'h' does not equal 'H'. So use tolower() or toupper() on word[i] when passing it to the strchr() function.

good

char c = 'y';
if (c == 'y' || c == 'Y')
    do_something()

better

char c = 'y';
if (toupper(c) == 'Y')
    do_something()

Lastly,
int index = c - word; is incorrect. A value you want is a number between 0-25 for the scores array, but this is not the way to do it. But rather int index = c - alphabet; and I'll tell you why.

When subtracting pointers like c - alphabet you get the difference in bytes. So, given a pointer char* p = strchr(alphabet, 'D'), then assume letter A is at address 200 and letter D is at 203 like so
A=200, B=201, C=202, D=203, etc. (remember alphabet = 200, the address of the first letter)
Then p - alphabet equals 3 (203 - 200 = 3)

[deceptikon]

dx9_programmer's answer is good, but I'd like to add just a couple of things. First, don't call strlen() in the condition of a loop because it's inherently inefficient. strlen() is a function that itself contains a loop, so when you say this:

for (int i = 0; i < strlen(word); i++) {
    ...
}

It's roughly equivalent to this:

for (int i = 0; ; i++) {
    int len = 0;

    for (int j = 0; word[j]; j++)
        ++len;

    if (i >= len)
        break;

    ...
}

If the length of word doesn't change in the body of the loop then it's silly to keep recalculating it. So calculate it before the loop, store it in a variable, then reference that variable whenever you need the length:

for (int i = 0, length = strlen(word); i < length; i++) {
    ...
}

Next is the actual cause of your segmentation fault. The problem is that c doesn't point to a location within word. Rather, c points to a location within alphabet. dx9_programmer explained the reason for the segmentation fault in a slightly confusing way, but I'd like to point out that it's also undefined behavior to take the difference of two pointers when they don't point to locations within the same array.

So while you might be getting a segmentation fault, that's not guaranteed to happen. Undefined behavior introduces 100% unpredictability to your program from the point it's invoked forward. So it's a good idea to avoid undefined behavior if only to avoid subtle and difficult to troubleshoot bugs. You actually got lucky this time because the bug caused an immediate and fatal error.

[HelloJarvis]

Great, thanks! A quick fix to the while loop got it right back up and running! Also, about the toupper and such, I omitted that code, so not to worry, it is being converted to uppercase.