6

I'm very new to C, and can't seem to figure out what's wrong with the following code.

int main() {
    char filen[] = "file.txt";
    FILE *file = fopen ( filen, "r" );
    if ( file != NULL )
    {
        char line [ 128 ];
        while ( fgets ( line, sizeof line, file ) != NULL ) /* read a line */
        {
            int i;
            char *result;
            for(i=0; i< NUM;i++)
            {
                char *rep;
                rep = (char *) malloc (sizeof(mychars[i][0]));
                strcpy(rep, mychars[i][0]);
                char *with;
                with = (char *) malloc (sizeof(mychars[i][1]));
                strcpy(with, cgichars[i][1]);
                result = (char *) malloc (sizeof(char) * 128);
                result = str_replace(line, rep, with);
            }


            fputs(result, stdout);
        }
    }
    fclose ( file );


    return 0;
}

Valgrind is giving me this error: 

==4266== Invalid read of size 1
==4266==    at 0x4C286D2: __GI_strlen (mc_replace_strmem.c:284)
==4266==    by 0x5118A8D: fputs (iofputs.c:37)
==4266==    by 0x400A0F: main (repl.c:35)
==4266==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

repl.c corresponds to the line beginning with fputs toward the end of this code.

Also, mychars is a two dimensional array that looks like this:

char *mychars[NUM][2] = {
  "a", "97",
  "b", "98",
  ....

Can someone please tell me how to fix this? Also, any pointers on how I should improve my current code (especially with malloc) would be much appreciated.

Edit: Code for str_replace

char *str_replace(char *str, char *orig, char *rep) {
  char buffer[4096];
  char *p;

  if(!(p = strstr(str, orig)))
    return NULL;

  strncpy(buffer, str, p-str);
  buffer[p-str] = '\0';
  sprintf(buffer+(p-str), "%s%s", rep, p+strlen(orig));

  return buffer;

}

EDIT 2 New Code for str_replace, and main

For testing purposes, I've replaced my str_replace method with the one found here:

What is the function to replace string in C?

And my main is changed slightly:

int main() {
    static const char filen[] = "file.txt";
    FILE *file = fopen ( filen, "r" );
    if ( file != NULL )
    {
        char line [ 128 ];
        while ( fgets ( line, sizeof line, file ) != NULL ) /* read a line */
        {
            int i;
            char *result;
            for(i=0; i< NUM;i++)
            {
                char *rep;
                rep = (char *) malloc (sizeof(mychars[i][0]));
                strcpy(rep, mychars[i][0]);
                char *with;
                with = (char *) malloc (sizeof(mychars[i][1]));
                strcpy(with, mychars[i][1]);
                result = str_replace(line, rep, with);
            }


            fputs(result, stdout);
        }
    }
    fclose ( file );


    return 0;
}

But I'm still getting

==6730== Invalid read of size 1
==6730==    at 0x4C286D2: __GI_strlen (mc_replace_strmem.c:284)
==6730==    by 0x5118A8D: fputs (iofputs.c:37)
==6730==    by 0x400995: main (repl.c:29)
==6730==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Perhaps the most frustrating part of this is not knowing what these Invalid read errors are.

EDIT 3 I've updated the code in the center of the for loop as such:

        int i;
        char* result;
        result = &line[0];
        for(i=0; i< NUM_CGICHARS;i++)
        {
            char *rep;
            rep = (char *) malloc (sizeof(char));
            strcpy(rep, cgichars[i][1]);
            char *with;
            with = (char *) malloc (sizeof(char)*3);
            strcpy(with, cgichars[i][0]);
            result = str_replace(result, rep, with);
            fputs(result, stdout);
            free(rep);
            free(with);
        }

And now I'm starting to get output! However, after only two iterations, I get a segmentation fault, with valgrind giving me a whole bunch of this:

==9130== Invalid read of size 1
==9130==    at 0x4C286D2: __GI_strlen (mc_replace_strmem.c:284)
==9130==    by 0x5118A8D: fputs (iofputs.c:37)
==9130==    by 0x4009DF: main (teststep1.c:27)
==9130==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
Community
  • 1
  • 1
varatis
  • 14,494
  • 23
  • 71
  • 114

3 Answers3

8

In these two lines

            result = (char *) malloc (sizeof(char) * 128);
            result = str_replace(line, rep, with);

you first allocate space for result that you then loose immediately after by overwriting it with the return of str_replace. That function probably returns 0, so your fputs fails.

BTW, don't cast the return of malloc, in C this is superfluous and may hide the fact that you forgot to include the prototype.

Edit: Your str_replace function is completely wrong in its memory handling. Never return the pointer to a local variable, the space isn't valid after you have left the function.

Jens Gustedt
  • 76,821
  • 6
  • 102
  • 177
  • Jens, thanks but I still have no idea how to fix this. Code for str_replace above – varatis Apr 12 '12 at 21:31
  • @varatis, for `str_replace` see my edit. For the logic of your program, I have a hard time to understand what you want to achieve. Why are you allocating something inside the loop that you then erase, and where you just would use the very last assigned value in the `fputs`? – Jens Gustedt Apr 12 '12 at 21:35
  • I'm trying to read a file in line by line, and for each line, replace certain characters of that line with strings specified by mychars – varatis Apr 12 '12 at 21:41
  • Jens, I've replaced my str_replace method with the one found in the link in the edit, so that I can properly debug my main() function. However, I'm still getting a similar error. – varatis Apr 12 '12 at 21:54
  • @varatis, you should definitively cut down your problem in smaller ones. Don't try to do several replacements in a `for` loop for the moment and for several lines, this is much too complex. Just do one such replacement without the `for` and `while` loop and try to output the result. Actually, replacement is already the wrong term, since you will not be able to do simple replacement of characters, your source and target strings seem to have different length. – Jens Gustedt Apr 12 '12 at 22:12
  • Jens, I've just one replacement, and it works fine. Actually, the program is able to get to two replacements with still working. View update 3 for new code. (Also the method is tested already for strings of different lengths, and it works) – varatis Apr 12 '12 at 22:24
0

If NUM is 0, then result is uninitialized and may be 0 by chance.

You aren't checking the results of your calls to malloc(), so a failure can mean you're trying to write to a NULL pointer.

Where is mychars declared?

Graham Borland
  • 60,055
  • 21
  • 138
  • 179
  • They're both declared in a "mychars.h" file, which is imported. But I've also noticed that this works except when I include str_replace... I'm going to go ahead and add that code – varatis Apr 12 '12 at 21:27
  • `malloc` returning `NULL` is very rare these days, especially with operating systems that implement memory overcommitting. – dreamlax Apr 12 '12 at 21:27
  • @James: Look up "memory overcommit" and "oom killer". One some systems, if you ask for memory from `malloc` (more than is available), `malloc` will return a valid pointer (virtual address), and it won't cause issues until you actually write to each page. Once you need to write to a page of memory that the operating system doesn't have, the OOM Killer will kill off processes based on a priority system to free up more pages for you to write to. Although, the OOM Killer can also kill off your own process if it deems it "low priority". – dreamlax Apr 12 '12 at 22:26
  • [Here's](http://opsmonkey.blogspot.com.au/2007/01/linux-memory-overcommit.html) a better explanation about it – dreamlax Apr 12 '12 at 22:29
  • That post isn't particularly complementary about it, and it seems to me a terrible idea. Just check for NULL and handle as appropriate. – James Apr 13 '12 at 00:41
  • @James: I'm not advocating it, I think it is a terrible idea also. Obviously checking for `NULL` is always the best approach, but just worth noting that not all operating systems return `NULL` when they should, so when your process dies it might not be from trying to use `NULL` but because the OOM Killer killed it instead. – dreamlax Apr 13 '12 at 02:11
0

You don't show how mychars is declared, but this line:

rep = (char *) malloc (sizeof(mychars[i][0]))

looks like it probably only allocates one byte. Also you're allocating a lot of memory and never freeing it. And here:

result = (char *) malloc (sizeof(char) * 128);
result = str_replace(line, rep, with);

You allocate memory using malloc and then completely discard the pointer to that memory by assigning the return value of another function over the top of it.

dreamlax
  • 93,976
  • 29
  • 161
  • 209