Segmentation fault in C txt file I/O

Question

Ok guys, the purpose of my program is to read from a text file called orginal.txt containing names in the format:

Kyle Butler
Bob Jones
Nathan Moore

The program then takes these names one at a time and turns them into something like:

Kyle.Butler@emailaddress.com

This address is then stored line by line in a new text file called final.txt

problem is, i can't get it to work, it gives me a segmentation fault and does not even get to writing to final.txt

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

void write(char line[100]);

int main()
{
    FILE *fp;
    fp = fopen("original.txt", "r+");

    char line[100];
    char mod[30]="@fakeemail.com\n";
    while (fgets(line, 100, fp) != NULL){
        int i;
        for(i=0; i<100; ++i){
            if(line[i]==' '){
                line[i]='.';
            }
            if(line[i]=='\n'){
                line[i]='\0';
            }
            strcat(line, mod);
        }



        FILE *fp2;
        fp2 = fopen("final.txt", "a");

        if (fp2 != NULL){
            fputs(line, fp2);
            fclose(fp2);
        }

    }

    fclose(fp);





    return 0;
}

Where is the segfault happening? On this line? strcat(line, mod); — canhazbits, Oct 06 '13 at 23:53
As an upgrade, you might want to consider whether you need to open and close the output file for every single write. — Duck, Oct 06 '13 at 23:54
Did you define a function of your own called `write`? There's already a C library routine called `write`. You should probably choose a name that's a little more unique to our application. `write` is pretty general. — lurker, Oct 06 '13 at 23:55
Always check the return value of fopen(). It's going to bite you sooner or later. — Charlie Burns, Oct 06 '13 at 23:57
And always check the return value of fclose() as well: http://stackoverflow.com/questions/19056309/not-checking-closes-return-value-how-serious-really — Jongware, Oct 07 '13 at 10:17

score 4 · Accepted Answer · answered Oct 06 '13 at 23:54

4

There are several problems with the code, but the segmentation fault is probably caused by this for loop:

for(i=0; i<100; ++i){
    if(line[i]==' '){
        line[i]='.';
    }
    if(line[i]=='\n'){
        line[i]='\0';
    }
    strcat(line, mod);
}

Every time through the loop you are concatenating mod to line. Since you iterate the loop 100 times with no other option to quit the loop, and line is only 100 characters long, very soon you will write past the 100th character of line into some other part of memory.

answered Oct 06 '13 at 23:54

lurker

56,987
9
69
103

+1 Fairly sure that `strcat` is supposed to be outside the loop, which would address what this answer points out as well. – WhozCraig Oct 06 '13 at 23:58
cheers, i remember seeing that and thinking it looked a bit out of place :) – Dylan Cleaver Oct 07 '13 at 00:16

paulsm4 · Answer 2 · 2013-10-07T00:17:25.347

Suggested changes:

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

#define MAXLEN 100   /* Somehow parameterize "100" */

void write(char line[100]);

int main()
{
    FILE *fp;
    fp = fopen("original.txt", "r+");
    if (!fp) {   /* Check for error */
      perror ("open failed");
      return 1;
    }
    char line[MAXLEN];
     /* You don't need hard-coded array length with a static string */
    char *mod="@fakeemail.com\n"; 
    while (fgets(line, MAXLEN, fp) != NULL){
        int i;
        /* You don't need to iterate through more characters than the string contains */
        for(i=0; i<strlen(line); ++i){
            if(line[i]==' '){
                line[i]='.';
            }
            if(line[i]=='\n'){
                line[i]='\0';
            }
        }
        /* Move this OUTSIDE of your for loop */
        strcat(line, mod);

        /* Append to the output file */
        FILE *fp2;
        fp2 = fopen("final.txt", "a");
        /* You're checking for error: good! */
        if (fp2 != NULL){
            fputs(line, fp2);
            fclose(fp2);
        }

    }

    fclose(fp);
    return 0;
}

You would really call `strlen()` in the conditional expression, as opposed to just walking the string with a local `char*` until the null-char is hit (and throwing out the 100-step for-loop in the process)? Or is this just the minimal-change you're targeting? — WhozCraig, Oct 07 '13 at 00:05

score -1 · Answer 3 · answered Oct 07 '13 at 00:18

As mbratch wrote you write beyond 100 chars in line array. Here's a working code:

void write(char line[100]);

int main()
{
    FILE *fp;
    fp = fopen("original.txt", "r+");

    char line[100];
    char mod[30]="fakeemail.com\n";
    while (fgets(line, 100, fp) != NULL){
        int i;
        for(i=0; i<100; ++i){
            if(line[i]==' '){
                line[i]='.';
            }
            if(line[i]=='\n'){
                line[i]='@'; strcat(line, mod);
                line[i+strlen(mod)]='\0';
            }
        }
        FILE *fp2;
        fp2 = fopen("final.txt", "a");

        if (fp2 != NULL){
            //fputs(line, fp2); printf("%s\n",line);
            fprintf(fp2,"%s\n",line);
            fclose(fp2);
        }
    }

    fclose(fp);
    return 0;
}

Segmentation fault in C txt file I/O

3 Answers3