Reversing String in C for loop error

Question

I have an array of strings and am trying to reverse each string in the array to see if that string is a palindrome. I am using a for loop to increment an int i (the index). However after the I call the reverse function, the value of i becomes some really large number and I cant figure out why this is happening.

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

void revString(char *dest, const char *source);

int main() {    
    const char *strs[] = {
        "racecar",
        "radar",
        "hello",
        "world"
    };

    int i;
    char res[] = "";
    for (i = 0; i < strlen(*strs); i++) {
        printf("i is %d\n", i);
        revString(&res[0], strs[i]); //reversing string
        printf("i is now %d\n", i); 

        //comparing string and reversed string  
        if (strcmp(res, strs[i]) == 0) {
            printf("Is a palindrome");
        } else {
            printf("Not a palindrome");
        }
    }
    return 0;
}

void revString(char *dest, const char *source) {
    printf("%s\n", source);
    int len = strlen(source);
    printf("%d\n", len);
    const char *p;
    char s;
    for (p = (source + (len - 1)); p >= source; p--) {
        s = *p;
        *(dest) = s; 
        dest += 1;
    }
    *dest = '\0';
}

This is the output showing the value of i before and after the revString function is called.

i is 0
i is now 1667588961
Illegal instruction: 4

regarding i < strlen(*strs): you probably want to compute the strlen() before the loop. — Andrew Jenkins
– Andrew Jenkins, Commented Jan 31, 2017 at 20:33
C does not support methods. Only functions. And none of the functions are methods. — too honest for this site
– too honest for this site, Commented Jan 31, 2017 at 20:35
char res[] = ""; make for a very small place to store a reversed string Only 1 char long`. — chux
– chux, Commented Jan 31, 2017 at 20:41
@AndrewJenkins Good point, but "i" is still jumping to a really large value — srsxyz
– srsxyz, Commented Jan 31, 2017 at 20:41

chqrlie · Accepted Answer · 2017-01-31 23:32:08Z

There are multiple problems in your code:

You pass a destination array char res[] = ""; that is much too small for the strings you want to reverse. It's size is 1. This causes buffer overflow, resulting in undefined behavior.

Use char res[20]; instead.
You enumerate the array of string with an incorrect upper bound. Use this instead:
```
for (i = 0; i < sizeof(strs) / sizeof(*strs); i++)
```
The termination test for the loop in revString() is incorrect too: decrementing p when is equal to source has undefined behavior, although it is unlikely to have an consequences. You can simplify this function this way:
```
void revString(char *dest, const char *source) {
    size_t len = strlen(source);
    for (size_t i = 0; i < len; i++) {
        dest[i] = source[len - i - 1];
    }
    dest[len] = '\0';
}
```

Here is the resulting code:

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

void revString(char *dest, const char *source) {
    size_t len = strlen(source);
    for (size_t i = 0; i < len; i++) {
        dest[i] = source[len - i - 1];
    }
    dest[len] = '\0';
}

int main(void) {
    const char *strs[] = { "racecar", "radar", "hello", "world" };
    char res[20];

    for (size_t i = 0; i < sizeof(strs) / sizeof(*strs); i++) {
        revString(res, strs[i]);
        //comparing string and reversed string  
        if (strcmp(res, strs[i]) == 0) {
            printf("Is a palindrome\n");
        } else {
            printf("Not a palindrome\n");
        }
    }
    return 0;
}

Dhruv Khatri · Accepted Answer · 2017-01-31 21:13:43Z

0

Here is Final Code with some change

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

void revString(char* dest, const char* source);
int main(){
    const char* strs[] = {
        "racecar",
        "radar",
        "hello",
        "world"
    };

    static int i;
    char res[] = "";
    int length = (int) sizeof(strs)/sizeof(char*);
    for(i = 0; i < length; i++)
    {
        printf("i is %d\n", i);
        revString(&res[0], strs[i]); //reversing string
        printf("i is now %d\n", i);

        //comparing string and reversed string
        if(strcmp(res, strs[i]) == 0){
            printf("Is a palindrome");
        }else{
            printf("Not a palindrome");
        }
    }
    return 0;
}
void revString(char* dest, const char* source){
    printf("%s\n", source);
    int len = (int) strlen(source);
    printf("%d\n", len);
    const char* p;
    char s;
    for(p = (source + (len - 1)); p >= source; p--){
        s = *p;
        *(dest) = s; 
        dest += 1;
    }
    *dest = '\0';

}

Change 1 :-

int i; to static int i; (Reason:- i is local variable you are calling function so when function call the value of i will remove and after that it will assign garbage value.)

change 2 :-

strlen(*strs) to length of array (because strlen(*strs) will give the length of first string)

answered Jan 31, 2017 at 21:13

Dhruv Khatri

8136 silver badges15 bronze badges

3 Comments

chqrlie Over a year ago

static int i;: absolutely not!

Dhruv Khatri Over a year ago

Why not? @chqrlie

chqrlie Over a year ago

Giving local variable i static storage does not fix the problem. It may hide it in some circumstances, but it does not correct the fact that the array res[] is too short. Furthermore, it is bad advice to let the OP believe local static variables are harmless. This construction should be reserved for cases where it is necessary and an API change is not possible or would not suffice. The problems with strtok() are rooted in this usage, avoid it whenever possible.

Collectives™ on Stack Overflow

Reversing String in C for loop error

2 Answers 2

Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related