n00b: junk output

[Kudose]

'ello all.

I am trying to get better with C and am taking one of laserlights suggestions by making a program to read a dictionary, find the palindromes and return the longest one.

Right now I am just trying to get my string reverse function to work.

It does reverse the string and return it, but there are junk values before and after the returned pointer.

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

char* strrev(char *str){
     int i = 0;
     int length = strlen(str);
     char *tmp = malloc(sizeof(char)*length);
     while(*(str+i) != '\0'){
       tmp[length - i] = *(str+i);
       i++;
     }
     return tmp;
}

int main(int argc, char* argv[]){

    char file[] = "c:\\dictionary.txt";
    FILE *fp = fopen(file, "r");
    if(fp != NULL){
      char *line = malloc(sizeof(char)*1024);
      int cnt = 0;
      while(fgets(line, 1024, fp) != NULL){
        printf("%s", line);
        printf("%s\n", strrev(line));
        ++cnt;
        if(cnt > 5) break;
      }
    } else {
      perror("Could not open the file ");
    }
    fclose(fp);
    system("PAUSE");
    return 0;
}

Sorry I can't post up the dictionary, it is like 15MB. You can create dictionary.txt with each word on a new line to test it out.

Any help is appreciated. Any criticism about how I could do this better would be appreciated as well.

[root4]

Your index is off by 1, so you're not filling tmp[0] and you're not setting the null char at the end (tmp[lenght]), resulting in junk at the beginning and at the end.

Also don't use sizeof(char), this is equal to 1 by definition.

EDIT: You are not freeing your strings.

[tabstop]

You need to null-terminate your new string, and your indexing is off when you write -- for instance, the first character you write is supposed to go to tmp[length], but there is no such place (tmp only goes out to length-1).

[Elysia]

     while(*(str+i) != '\0'){

This is the same as while(str[i] != '\0').

        printf("%s\n", strrev(line));

This is the worst possible thing to do EVER. Strrev returns malloced memory and you never store it somewhere so you can't free it.
That results in a memory leak.
You must free everything allocated with malloc.

[matsp]

You are returning a variable that is local to the function you are in. That means that the memory you are storing the string in will be released and reused for other purposes after you return from the function. So your string will be overwritten with other things when you call printf().

The most obvious suggestions would be:
1. Reverse the input string by swapping the corresponding positions in situ.
2. Pass a second string of sufficient size to fill in as the output string.
3. Allocate memory to store the new string in. Just don't forget to free it afterwards. In this case, that's a pretty poor solution, but it's a possibility.

--
Mats

[Kudose]

Thanks you guys n gals!

Where would I free the strings? (edit: let me try to figure this out with the two new posts above)

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

char* strrev(char *str){
     int i = 0;
     int length = strlen(str); // 3
     char *tmp = malloc(length);
     while(*(str+i) != '\0'){
       tmp[length - (i + 1)] = *(str+i);
       i++;
     }
     tmp[length] = '\0';
     return tmp;
}

char* stripNewline(char* str){
      int i = 0;
      while(*(str+i) != NULL){
        if(*(str+i) == '\n')
             *(str+i) = '\0';
        i++;
      } 
      return str;
}

int main(int argc, char* argv[]){
    char file[] = "c:\\dictionary.txt";
    FILE *fp = fopen(file, "r");
    if(fp != NULL){
      char *line = malloc(sizeof(char)*1024);
      int cnt = 0;
      while(fgets(line, 1024, fp) != NULL){
        printf("&#37;s\n", stripNewline(line));
        printf("%s\n", strrev(line));
        ++cnt;
        if(cnt > 5000) break;
      }
    } else {
      perror("Could not open the file ");
    }
    fclose(fp);
    system("PAUSE");
    return 0;
}

[Kudose]

Ok, I freed my precious memory.

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

char* strrev(char *str){
     int i = 0;
     int length = strlen(str); // 3
     char *tmp = malloc(length);
     while(*(str+i) != '\0'){
       tmp[length - (i + 1)] = *(str+i);
       i++;
     }
     tmp[length] = '\0';
     return tmp;
}

char* stripNewline(char* str){
      int i = 0;
      while(*(str+i) != NULL){
        if(*(str+i) == '\n')
             *(str+i) = '\0';
        i++;
      } 
      return str;
}

int main(int argc, char* argv[]){
    char file[] = "c:\\dictionary.txt";
    FILE *fp = fopen(file, "r");
    if(fp != NULL){
      char *line = malloc(sizeof(char)*1024);
      int cnt = 0;
      while(fgets(line, 1024, fp) != NULL){
        line = stripNewline(line);
        char* tmp = strrev(line);
        printf("&#37;s\n", line);
        printf("%s\n", tmp);
        free(tmp);
        ++cnt;
        if(cnt > 5000) break;
      }
    } else {
      perror("Could not open the file ");
    }
    fclose(fp);
    system("PAUSE");
    return 0;
}

## edit ## looks like I took your 3rd solution, even though it wasn't the best. I will rewrite this taking all of your feedback into consideration.

[swoopy]

> char *tmp = malloc(length);
As tabstop pointed out, you allocate enough space for the string, but not the string terminator. And in main() you should also free the memory for line once you're done.