Allocating memory from a file, then using fread and fwrite C

Question

I'm having a bit of trouble getting my code to work, which is to open a file, count the number of characters in it, and then allocating that using malloc(). And then I am supposed to read the characters in from one file (mine contained "Hello World!") using fread(), and write them to a blank .txt file using fwrite.

My code so far is printing corrupted characters. I couldn't find any questions that were specific enough to my problem. If anyone could tell me what I'm doing wrong I'd appreciate it. I think it is specific to my fread and fwrite calls, but nothing I've tried works.

The code in question (not commented yet, sorry!):

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

//initialized using ./a.out in.txt out.txt

int main(int argc, char *argv[])
{
    FILE *fp;
    int count, end;
    char *memory;
    char c[64]; 

    fp = fopen(argv[1], "r");

    if((fp) == NULL)
    {
        printf("Error: cannot open file.\n");
    }

    else
    {
        while((fgetc(fp))!= EOF)
        {
            count++;    
        }

        memory = (char*)malloc(count);

        c[64] = fread(memory, sizeof(char), 1, fp);
        fclose(fp);

        fp = fopen(argv[2], "w");
        fwrite(c, sizeof(char), sizeof(c), fp); 

        fclose(fp);     
        free(memory);
    }       
    return 0;   
}

Once you read the file once, you have to rewind it before you can reread it. (You forgot to initialize `count` to zero before you start.) You didn't check that the memory allocation was successful. You can't write to `c[64]` reliably since `c[63]` is the last element of the array. You only attempt to read one byte, not `count` bytes. The value returned by `fread` is the number of bytes read. That should be the number you write, not the size of the array. You read the data into `memory` but you write the garbage from uninitialized array `c`. That's enough to give you problems. — Jonathan Leffler, Dec 09 '15 at 07:20
Also, minor points: `sizeof(char)` is redundant (just use `1`), and [casting the result of `malloc` is unnecessary and potentially dangerous in C](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — Paul R, Dec 09 '15 at 07:22
@JonathanLeffler To be more correct the value returned by `fread` is the number of **items** read. — skyking, Dec 09 '15 at 07:34
Be aware that the size of the file may have changed the second time you read it. — skyking, Dec 09 '15 at 07:36
@skyking: Yes. There are disadvantages to comments — like they have limited space and you can't edit them indefinitely. — Jonathan Leffler, Dec 09 '15 at 07:36
After your `while` loop your `fp` is already at `EOF`! And I guess you don't want to read only 1 byte, but `count` with that `fread`. Same goes for `fwrite`. Look up `fseek` on how to set the file pointer to the beginning of the file again after your while loop. — René Vogt, Dec 09 '15 at 07:36
Yeah.. there ought to be close reason for 'so many bugs/problems that it's not easily possible to pick out one to give a fix to'. — Martin James, Dec 09 '15 at 07:44

Mohan · Accepted Answer · 2015-12-09T09:02:21.497

3

code have logical mistakes, as follow

Initialise variables int count= 0 , char c[64]= {0};
Type cast not required memory = malloc(count);
First you have counted number of char in file so Before reading again file rewind it by fseek(fp,0,SEEK_SET);
c[64] = fread(memory, sizeof(char), 1, fp); In this if you are reading single char, you should read complete file , To read complete file do fread(memory, 1, count, fp); and c[64] is out of bound and fread return the number of char successfully read .
fwrite(c, sizeof(char), sizeof(c), fp); In this you are writing complete char array to file but you have read only single variable in array which number of char read . So you are writing uninitialised char array to file. so you are getting corrupted character in file.
To write in file do fwrite(memory, 1, count, fp);

To solve problem ,avoid above error and read complete file in char array and then write.

edited Dec 09 '15 at 09:02

answered Dec 09 '15 at 07:43

Mohan

1,871
21
34

`sizeof(char)` equals 1 by definition, therefore it can be replaced safely by `1`. – Jabberwocky Dec 09 '15 at 07:47
Since the variable `end` is unused, it should be deleted, not initialized. – Jonathan Leffler Dec 09 '15 at 08:10
Thank you so much for the help! I realized most were simple mistakes, but I finally understand how to use fread and fwrite. Thank you! – Orion96 Dec 09 '15 at 21:53

score 1 · Answer 2 · edited May 23 '17 at 12:30

Alright, there are a number of problems here, I'll start with the least bad:

memory = (char*)malloc(count);

Casting the return of malloc() is unnecessary and can potentially mask errors, for more info see here.

int count;

You never initialise count to anything. This is undefined behaviour and there is no guarantee it'll start at 0. It can start at random garbage left in memory. Same for end

    c[64] = fread(memory, sizeof(char), 1, fp);

2 Problems here. c[64] is out of bounds for the array c since indexes start at 0, so the last element in the array is c[63]. sizeof(char) is defined to be 1, so use 1 instead. Further, fread() returns the amount of characters read, so not sure what you are trying to do with that value even.

fwrite(c, sizeof(char), sizeof(c), fp);

You're writing a complete uninitialised array to the file (=garbage)

Allocating memory from a file, then using fread and fwrite C

2 Answers2