3

I try to copy files using this function, but the output files contains strange characters.

int File_Copy (char FileSource [], char FileDestination [])
{
    int     result      =   -1;
    char    c [1];
    FILE    *stream_R   =   fopen (FileSource,      "r");
    FILE    *stream_W   =   fopen (FileDestination, "w");   //create and write to file

    while ((c [0] = (char) fgetc(stream_R)) != EOF)
    {
        fprintf (stream_W, c);
    }

    //close streams
    fclose  (stream_R);
    fclose  (stream_W);

    return result;
}

I do not know what is wrong. Please help.

CaTx
  • 1,421
  • 4
  • 21
  • 42
  • [`fgetc()`](http://port70.net/~nsz/c/c11/n1570.html#7.21.7.1) returns a value of type `int`. Converting it to `char` loses information. – pmg Mar 16 '15 at 14:15

2 Answers2

7

The problem is that c[1] will not work as a string, because it can't contain the terminating nul byte, so it should be

char c[2] = {0};

and also c[2] should be int, like this

int c[2] = {0};

because fgetc() returns int so your code is potentially overflowing c[0], but you also have some other things you can improve.

  1. You don't need c to be an array, you can just declare it like this.

    int c;

    and then use fputc(); instead of fprintf().

  2. You must check that none of the fopen() calls failed, otherwise your program will invoke undefined behavior because of NULL pointer dereference.

This is a robust version of your own program with the problem you describe in your question fixed

/*   ** Function return value meaning
 * -1 cannot open source file 
 * -2 cannot open destination file
 * 0 Success
 */
int File_Copy (char FileSource [], char FileDestination [])
{
    int   c;
    FILE *stream_R;
    FILE *stream_W; 

    stream_R = fopen (FileSource, "r");
    if (stream_R == NULL)
        return -1;
    stream_W = fopen (FileDestination, "w");   //create and write to file
    if (stream_W == NULL)
     {
        fclose (stream_R);
        return -2;
     }    
    while ((c = fgetc(stream_R)) != EOF)
        fputc (c, stream_W);
    fclose (stream_R);
    fclose (stream_W);

    return 0;
}
Iharob Al Asimi
  • 52,653
  • 6
  • 59
  • 97
5

Is there any reason why you are trying to copy the file one byte at a time? That is going to be so slow! Although your main problem is probably that you use fprintf(), and the printf() functions are intended for printing formatted strings, not for individual characters.

If you're just pushing bytes around from one file to another, then you should use fread and fwrite instead, like so:

int File_Copy(char FileSource[], char FileDestination[])
{
    char    c[4096]; // or any other constant you like
    FILE    *stream_R = fopen(FileSource, "r");
    FILE    *stream_W = fopen(FileDestination, "w");   //create and write to file

    while (!feof(stream_R)) {
        size_t bytes = fread(c, 1, sizeof(c), stream_R);
        if (bytes) {
            fwrite(c, 1, bytes, stream_W);
        }
    }

    //close streams
    fclose(stream_R);
    fclose(stream_W);

    return 0;
}
Enno
  • 1,736
  • 17
  • 32
  • I did not know of the other functions. your way seems to have a constraint on file size (size of c[]). is there a way to modify it to remove the constraint? – CaTx Mar 16 '15 at 17:49
  • This version is faster because it reads 4096 bytes at a time. If you want to read 1 byte at a time then change 4096 to 1. Also I/O error handling is needed, like in iharob's answer. – Barmak Shemirani Mar 16 '15 at 20:43
  • @CaTx There is no constraint on file size. The code copies 4096 bytes at a time, in a loop, until all the data has been copied. This is somewhat faster than copying a byte at a time, at the expense of a little more memory usage (but what is 4 KB these days?) – Enno Mar 19 '15 at 13:40