C program that returns the initials of a name (CS50)

Question

Here's my main

char* name = GetString();
if(name != NULL)
{
    for(int i = 0, j = strlen(name); i < j; i++)
    {
        if(!isalpha(name[i-1]) && isalpha(name[i]))
            printf("%c", toupper(name[i]));
    }
    printf("\n");
}

The program works pretty good; it passed all the (check50) tests.

But I'm worried about a possible bug and that is when i = 0, could the value stored in name[-1] be an alpha character?

Note: the GetString() function returns a string entered by the user.

see http://stackoverflow.com/questions/15646973/how-dangerous-is-it-to-access-an-array-out-of-bounds for detailed explanations on why it is wrong to access memory that is not yours. — Selçuk Cihan, Dec 28 '15 at 16:19
Does the `GetString` function guarantee that the character prior to the first is accessible and not alphanumeric? If so, this code is fine. If not, this code is broken. — David Schwartz, Dec 28 '15 at 16:21
You could use `if ((i == 0 || !isalpha(name[i-1])) && isalpha(name[i]))` as the condition in the loop. — Jonathan Leffler, Aug 07 '16 at 05:50

haccks · Answer 1 · 2015-12-28T16:41:06.717

5

For i = 0, name[i-1] is accessing unallocated memory and it will lead to undefined behavior. Change loop counter to i = 1 instead of i = 0.

As mentioned in comments you need to check the condition for i =0 outside the loop.

edited Dec 28 '15 at 16:41

answered Dec 28 '15 at 16:18

haccks

104,019
25
176
264

1

and check condition for `i = 0` separately before the loop. – Haris Dec 28 '15 at 16:22
1

If he wants to check whether `name[0]` is alpha or not. Changing what you have advised will skip that check – Haris Dec 28 '15 at 16:25
@haccks what Haris said is right. you're skipping name[0] – Harout Tatarian Dec 28 '15 at 16:35
@Haris - Haccks has it right. For `i == 0`, `name[i-1]` ( `name[0-1]` ) yields a negative array index. – ryyker Dec 28 '15 at 18:32
@ryyker, OP's algorithm is such for normal `i` he wants to check both and `i` and `i-1`, but think what he would want to do when `i-1` doesn't exist, like when `i=0`. – Haris Dec 28 '15 at 18:34
@Haris - If that is what he wants to do, then he must initialize the first ***i*** in the for loop to 1, not zero, to prevent a negative array index. That is exactly what Haccks is saying. He must also set the exit condition in for loop to leave at a value 1 less than the length of the array to prevent accessing un-owned memory at the other end. – ryyker Dec 28 '15 at 18:40

jweyrich · Answer 2 · 2015-12-28T16:25:22.903

3

Supposing the memory allocated for the string and pointed by name starts at name[0], then name[i-1] will cause an out-of-bound array access when i=0, which is undefined behaviour in C, and may cause all kinds of weird symptoms in your program (including it working properly :-). It's an unfortunate case that is passes all the tests, because it's misleading you.

edited Dec 28 '15 at 16:25

answered Dec 28 '15 at 16:20

jweyrich

31,198
5
66
97

score 2 · Accepted Answer · edited Aug 07 '16 at 05:44

2

Actually in C, the [] operator can be replaced with *().

a[n] is equal to *(a+n);

So, in your case, when i = 0, you're accessing *(a - 1) location while your string begins from a to a + length. So, this would result in unknown behavior.

+----+----+----+----+
| f  |  o | o  | \0 |
+----+----+----+----+
  ^              ^
  |              |
  a            a + 3

edited Aug 07 '16 at 05:44

Jonathan Leffler

730,956
141
904
1,278

answered Dec 28 '15 at 16:43

frogatto

28,539
11
83
129

C program that returns the initials of a name (CS50)

3 Answers3