Hexdump text and binary files in C

Question

I am writing a code in C performing hexdump in both text and binary files. My output in text files are correct but when I tried performing hexdump in a binary file, I got garbages. I would like to ask for your help if which part of my code is wrong and how should I correct my mistakes. Thanks.

#include <stdio.h>
#define OFFSET 16

main(int argc, char const *argv[]) 
{
    FILE *fp;
    char buff[OFFSET];
    int read;
    int address = 0;
    int i;

    if (argc != 2){
        exit(0);
    }

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

    while ((read = fread(buff, 1, sizeof buff, fp)) > 0){
        printf("%08x ", address);
        address += OFFSET;

        //print hex values 
        for (i = 0; i < OFFSET; i++){

            if(i >= read){
                buff[i] = 0;
            }
            if(buff[i] >= 33 && buff[i] <= 255 || buff[i] != 00){
                printf("%02x ", buff[i]);
            }
            if(buff[i] == 00){
                printf("   ");
            }
        }

        //print ascii values
        for (i = 0; i < OFFSET; i++) {
            printf("%c", (buff[i] >= 33 && buff[i] <= 255 ? buff[i] : ' '));
        }
        printf("\n");
    }

    fclose(fp);
}

Answer 1

You have a couple of errors in logic. First as specified in the comment, all characters are equally important within a binary file. There is no need (and you shouldn't) test if(buff[i] >= 33 && buff[i] <= 255 || buff[i] != 00) for your binary output.

The proper declarations for main are int main (void) and int main (int argc, char **argv) (which you will see written with the equivalent char *argv[]). See: C11 Standard §5.1.2.2.1 Program startup p1 (draft n1570). See also: See What should main() return in C and C++?

Next with your binary output, you are attempting to print an unsigned value with %02x, but your are passing a signed character. If the char value is negative, you are trying to output the sign-extended value with outputs the full width of the unsigned value (02x will pad the field to 2 characters, but does not prevent more than two characters from printing). You have a couple of options, first use the hh length modifier to limit the type to 1-byte, and second simply cast the value to (unsigned char), e.g.

            printf("%02hhx ", (unsigned char)buff[i]);

You logic is also a bit cumbersome. You should use if ... else if ... else to handle your binary cases. Further, you are outputting two spaces when either i >= read || buff[i] == 0, so you may as well combine the test.

A short rewrite could look something like the following (which will read from the file given as the 1st argument -- or from stdin if no argument is given)

#include <stdio.h>

#define OFFSET 16

int main (int argc, char const *argv[]) 
{
    char buff[OFFSET] = "";
    int read, address = 0, i;
    FILE *fp = argc > 1 ? fopen (argv[1], "rb") : stdin;

    if (!fp) {
        perror ("fopen");
        return 1;
    }

    while ((read = fread(buff, 1, sizeof buff, fp)) > 0) {
        printf("%08x ", address);
        address += OFFSET;

        for (i = 0; i < OFFSET; i++)    /* print hex values */
            if (i >= read || buff[i] == 0)
                printf("   ");
            else
                printf("%02hhx ", (unsigned char)buff[i]);

        fputs ("| ", stdout); /* optional separator before ASCII */

        for (i = 0; i < OFFSET; i++)    /* print ascii values */
            printf("%c", (buff[i] >= ' ' && buff[i] <= '~' ? buff[i] : ' '));
        putchar ('\n'); /* use putchar to output single character */
    }

    if (fp != stdin)
        fclose (fp);
}

(note: if your compiler does not support the hh prefix, the cast itself will suffice)

Look things over and let me know if you have further questions.