Unwanted characters in output from fread.

[CodeWarrior14]

I am writing a program that reads a fiel and displays the contents in hex values.
The problem is that there are some very strange characters in my output.

Code:

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

#define MAX_BUFFER_LEN 512

typedef enum cbool { ctrue = 1, cfalse = 0 } cbool;

struct operation_info
{
    char *infilename;
    char *outfilename;
    cbool out;
} op_info;

int dump_file()
{
    FILE *infile;
    FILE *outfile;
    char buffer[MAX_BUFFER_LEN];
    int times;
    int fsize;
    int counter;
    int fcount;

    // Create the input file.
    infile = fopen(op_info.infilename, "rb");
    if (infile == NULL)
    {
        perror("Could not open file for reading!");
        return -1;
    }

    // Create the ouput file if needed.
    if (op_info.out == ctrue)
    {
        outfile = fopen(op_info.outfilename, "wb");
        if (outfile == NULL)
        {
            perror("Could not open file for writing!");
            return -1;
        }
    }

    // Get the size of the file.
    fseek(infile, 0, SEEK_END);
    fsize = ftell(infile);
    fseek(infile, 0, SEEK_SET);

    // Get how many times need to be read from the file.
    if ((fsize < MAX_BUFFER_LEN) || (fsize == MAX_BUFFER_LEN))
        times = 1;
    else
        times = fsize / MAX_BUFFER_LEN + 1;

    // Read from the file.
    counter = 0;
    while (counter < times)
    {
        fread(buffer, 1, MAX_BUFFER_LEN, infile);
        counter++;

        if (op_info.out == ctrue)
        {
            for (int i = 0; i < strlen(buffer); i++)
                fprintf(outfile, "%c", buffer[i]);
            fflush(outfile);    // Make sure everything is written to the file.
        }

        fcount = 0;
        for (int i = 0; i < strlen(buffer); i++)
        {
            printf("%.2x ", buffer[i]);

            if (fcount == 16)
            {
                printf(":  ");

                for (int k = i - 16; k < i; k++)
                {
                    if (((int)buffer[k] >= 32) && ((int)buffer[k] <= 126))
                        printf("%c", buffer[k]);
                    else
                        printf(".");
                }

                printf("\n");

                fcount = 0;
            }

            fcount++;
        }
    }

    // Close the file.
    fclose(infile);
    if (outfile)
        fclose(outfile);

    return 0;
}

int main(int argc, char **argv)
{
    op_info.infilename = "C:\\Users\\User\\Desktop\\hexdump.c";
    op_info.out = ctrue;
    op_info.outfilename = "C:\\Users\\User\\Desktop\\hexdump.txt";
    dump_file();
    getchar();

    return 0;
}

Sample output of strange characters (not all the output):

// Create the input file.
    infile = fopen(op_info.infilename, "rb");
    if (infile == NULL)
    {
        perror("Could not open file ÌÌÌÌÌÌÌÌpsLÌÌÌÌÌÌÌÌPsLÌÌÌ̬ãÁœú©for reading!");
        return -1;
    }

How do I get rid of all those ÌÌÌÌÌÌÌÌpsL characters?

Also, when ever I am printing the contents of buffer, some extra data that are left behind from the last time something was read into it also prints. How do I 'clean' the buffer so that only the current contents are in it?

Sample output:

int main(int argc, char **argv)
{
    op_info.infilename = "C:\\Users\\User\\Desktop\\hexdump.c";
    op_info.out = ctrue;
    op_info.outfilename = "C:\\Users\\User\\DesktÌÌÌÌÌÌÌÌpsLÌÌÌÌÌÌÌÌPsLÌÌÌ̬ãÁœú©op\\hexdump.txt";
    dump_file();
    getchar();

    return 0;
} <!-- This is suppose to be the end, but then the code from below shows up. -->
)buffer[k] >= 32) && ((int)buffer[k] <= 126))
                        printf("%c", buffer[k]);
                    else
                        printf(".");
                }

                printf("\n");

                fcount = 0;
            }

Also, how can I check if a file has been opened or not. Because:

if (outfile)
    fclose(outfile);

causes an error if I set op_info.out = ctrue; to op_info.out = cfalse;

Thanks in advance.

[Kanoisa]

Hi,

I would make the following suggestions to help you out.

Garbage data - char buffer[MAX_BUFFER_LEN]; gives you uninitialised memory - it could contain anything. so using strlen on it is not reliable, fread will not null terminate the string for you and you risk running off the end of memorywhich is a programming flaw and a potnetial security flaw.

How to initialise a block of memory ? - memset( pBuff, 0, sizeof( pBuff ) )

I would also suggest using the return value of fread() instead of manually loosely calculating a loop counter. fread returns the number of bytes read into your buffer. so instead of using your counter and times variables just decrement from the file size the number of bytes read from fread each time until the value reaches 0. YOu would then use the return value from fread in your loop to print to the output file too. By using the return value of fread instead of strlen you avoid the need to clear the buffer as your code will only process as many bytes as it got from the file.

Also where you print the values as hex you will get strange results. you store the binary byte value in a char buffer. char is a signed type so so the byte 0xFF will be seen as -1 and not 255. this means when given to printf or similar function it will be displayed wrong. This is because for hex printing printf will print an integer value. so your 0xFF byte is interpreted as -1 and then promoted to an integer, so in this case you would likely get 0xFFFF printed instead of 0xFF . the quick fix to this is to either change buffer to an unsigned char. or to typecast it to an unsigned char in your printf call.

Finally when you ask about the file close on the output file. Again your dealing with uninitialised memory here. if you choose not to open outfile the value is garbage - whatever was in memory before it was assigned to store outfile. so your check if( outfile ) will likely always be true but contain a value that is not a pointer to a file and this is why you get an error. Your options here are : initialise everything to a safe value before they are used ( reccommended ) or if op_info.out = cfalse; set outfile to false. there are other solutions but they would be hacks and i would not reccommend any other approach. In general always always always initialise every variable to a safe value as soon as you can - preferably at declaration.

[Dervish1]

Also don't forget that there's no guarantee the data written by fread doesn't have a null in it. So that's another reason that strlen is a bad way to figure out the buffer's length. (And to follow the above advice to use fread's return value).