Binary File Reading

The goal of the program is to read in any file, exe, mp3, etc. and make a working copy of it. Here's my code:

Code:

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

#define BUFFER 1
#define INPUT input
#define OUTPUT output

int main(int argc, char *argv[]) {
    
    if(argv[1] == NULL) {
        printf("Format:\n");
        printf("Program.exe input.z output.z\n");
        return 0;
    }
    if(argv[2] == NULL) {
        printf("Format:\n");
        printf("Program.exe input.z output.z\n");
        return 0;
    }
    if(argv[3] != NULL) {
        printf("Format:\n");
        printf("Program.exe input.z output.z\n");
        return 0;
    }
        
    
    FILE* INPUT;
    INPUT = fopen(argv[1], "rb");
    
    FILE* OUTPUT;
    OUTPUT = fopen(argv[2], "w");

    
    while(!feof(INPUT)) {
        
        unsigned char* buf = malloc(sizeof(char) * BUFFER);
        
        fread(buf, BUFFER, 1, INPUT);

        int i;
        for(i = 0; i < BUFFER; i++)
            fprintf(OUTPUT, "%c", buf[i]);
        
        free(buf);
    }
    
    fclose(INPUT);
    fclose(OUTPUT);
    
    return 0;
    
}

I viewed both the original and the copy in a hex editor and I saw that the copy had several extra 0Ds (hex). There are three possibilities for the problem that I can visualize, I'm reading in the input file incorrectly, I'm outputting the data incorrectly, or both. This problem has stumped me for a while now and I need some help. I hope you guys can help, Thank you!!

1. use argc to check the number of arguments. Don't probe each argv[] in turn.

2. You need to open the output file in "wb" mode.

3. malloc and free INSIDE the loop is a waste of effort. It's even more a waste of effort since it's for only ONE character.

4. Read the FAQ for information on why using feof() to control a loop is bad.

Salem,

1. I'll look into that, I didn't really give it much thought.
2. THANK YOU!
3. lol, yeah
4. didn't know that, i'll check out the faq

THANK YOU!

EDIT: Curse you Salem, and your speedy, concise methods of conveying information!

Don't #define INPUT as input. Identifiers that vary only in case are frowned upon. It's too easy to get confused. If you want to use all upper case, just declare the variable that way, and remove the #defines. But most would consider that bad style, since all upper case is typically reserved for constants. Just user all lowercase input, or input_file and likewise for output.

You could improve the handling of your command line parameters by simply checking if argc is equal to 3:

Code:

if (argc != 3) {
    printf("Format...");
    return 1; // usually 0 means the program ran successfully, non-zero means there was a problem
}

Here's a little more info if you want it: FAQ > Accessing command line parameters/arguments - Cprogramming.com.

You need to check the return value of fopen (both calls) and if it failed, print an error and quit:

Code:

FILE *input = fopen(...);
if (input == NULL) {
    perror("fopen of input file failed");
    return 1;
}

Then, read the man page for fopen (fopen(3): stream open functions - Linux man page). Read up on the modes, you're not opening in binary mode. The 0x0D are carriage returns (are you on Windows?) that are a result of "translating" line endings in text mode files.

Also, there's no need to malloc and free each time in the loop. You can malloc once before the loop and free after it's done, overwriting all the data. Better yet, don't malloc/free at all. Simply declare a character and read into/write from it's address:

Code:

unsigned char data;
...
if (fread(&data, sizeof(data), 1, input) != 1)
    // error, do something about it
...
if (fwrite(&data, sizeof(data), 1, output) != 1)
    // error, do something about it

As for the extra data, don't use feof to control a loop. Read this: FAQ > Why it's bad to use feof() to control a loop - Cprogramming.com.

That should cover most, if not all, of your problems.

anduril462,

The links you provided were very helpful, I made the edits you and Salem recommended. Thank you guys very much, now it's time to tackle a major beast since this test code is now working 100%!

Here is my version that I wrote a few weeks ago:

Code:

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

#define FILENAMELENGTH 30
#define LINELENGTH 4096

int main(void)
{
    FILE *openfile, *openfile2;
    char file1[FILENAMELENGTH], file2[FILENAMELENGTH], line[LINELENGTH];

    printf("Enter file to copy: ");
    fgets(file1, FILENAMELENGTH, stdin);
    file1[strlen(file1) - 1] = '\0';

    if((openfile = fopen(file1, "rb")) != NULL)
       printf("%s opened successfully\n", file1);
    else
    {
        perror("Error");
        exit(1);
    }

    printf("Enter file to copy to: ");
    fgets(file2, FILENAMELENGTH, stdin);
    file2[strlen(file2) - 1] = '\0';

    if((openfile2 = fopen(file2, "wb")) != NULL)
        printf("%s opened successfully\n", file2);
    else
    {
        perror("Error");
        exit(1);
    }

    size_t n;

    while((n = fread(line, 1, LINELENGTH, openfile)) > 0)
        if((fwrite(line, 1, n, openfile2)) != n)
        {
            puts("Error writing to file");
            exit(1);
        }

    fclose(openfile);
    fclose(openfile2);

    return 0;
}