Checksum a Game Boy cartridge

Question

I wrote a program to insert a checksum into a Game Boy cartridge. Here is a spec of the cartridge header, but I'll include the relevant information here, as well.

The header checksum is defined as the sum of the bitwise-NOT of the 52nd through the 76th bytes of the file, inclusive.

The global checksum is defined as the sum of all the bytes in the file except for the bytes of the global checksum itself.

This program requires placeholder zero bytes for these checksums. If the placeholder bytes are not zero, the program errors.

Here's the program:

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

// Insert various checksums into a Game Boy cartridge file.
// Gives an error if the checksums are present prior to checksumming.

#define HEADER_DATA \
/* nop; jmp $150 */ \
"\x00\xc3\x50\x01" \
/* Nintendo logo */ \
"\xce\xed\x66\x66\xcc\x0d\x00\x0b\x03\x73\x00\x83\x00\x0c\x00\x0d" \
"\x00\x08\x11\x1f\x88\x89\x00\x0e\xdc\xcc\x6e\xe6\xdd\xdd\xd9\x99" \
"\xbb\xbb\x67\x63\x6e\x0e\xec\xcc\xdd\xdc\x99\x9f\xbb\xb9\x33\x3e"

enum cartridge_header_section_sizes
{
    // oversimplification, but the extra data is not needed
    ENTRY = 0x0,
    MISC = ENTRY + 0x34,
    HEADER_CHECKSUM = MISC + 0x19,
    GLOBAL_CHECKSUM = HEADER_CHECKSUM + 0x01,
};

// checksum functions save and restore file position
static unsigned char header_checksum(FILE *const fp, size_t nbytes)
{
    fpos_t pos;
    unsigned char result;

    if(fgetpos(fp, &pos) == -1)
        return -1;

    for( result = 0; nbytes--; )
    {
        int c = fgetc(fp);

        if(c == -1)
            return -1;

        result += (unsigned char)~(unsigned char)c;
    }

    if(fsetpos(fp, &pos) == -1)
        return -1;

    return result;
}

static unsigned short global_checksum(FILE *const fp, size_t nbytes)
{
    fpos_t pos;
    unsigned short result;

    if(fgetpos(fp, &pos) == -1)
        return -1;

    for( result = 0; nbytes--; )
    {
        int c = fgetc(fp);

        if(c == -1)
            return -1;

        result += (unsigned char)c;
    }

    if(fsetpos(fp, &pos) == -1)
        return -1;

    return result;
}

int main(const int argc, const char *const *const argv)
{
    if(argc != 2)
    {
        printf("Usage: %s gb-file\n", argv[0]);
        goto fail;
    }

    FILE *const fp = fopen(argv[1], "r+b");
    if(fp == NULL)
    {
        perror(argv[1]);
        goto fail;
    }

    // -1 for the trailing null byte
    unsigned char header[sizeof(HEADER_DATA) - 1];
    if(fread(header, 1, sizeof(header), fp) != sizeof(header))
    {
        fputs("Short file: header read failed\n", stderr);
        goto fail;
    }

    if(memcmp(header, HEADER_DATA, sizeof(header)) != 0)
    {
        fputs("Invalid header!\n"
              "Make sure that your header contains nop; jp $150 "
              "and the official Nintendo logo before running again.\n",
              stderr);
        goto fail;
    }

    errno = 0;
    unsigned char hchk = header_checksum(fp, HEADER_CHECKSUM - MISC);
    if(hchk == (unsigned char)-1 && errno)
    {
        perror("header checksum");
        goto fail;
    }
#ifndef NDEBUG
    printf("%hx\n", hchk);
#endif
    if(fseek(fp, HEADER_CHECKSUM, SEEK_SET) == -1)
    {
        perror("fseek");
        goto fail;
    }
    // sanity checking
    if(fgetc(fp))
    {
        fputs("Header checksum already in place!\n"
              "Did you run the program twice?\n", stderr);
        goto fail;
    }
    // rewind to previous position if successful
    if(fseek(fp, -1, SEEK_CUR) == -1)
    {
        perror("fseek");
        goto fail;
    }
    if(fwrite(&hchk, 1, sizeof(hchk), fp) != sizeof(hchk))
    {
        fputs("Insertion of header checksum failed.\n", stderr);
        goto fail;
    }

    if(fseek(fp, 0, SEEK_END) == -1)
    {
        perror("fseek");
        goto fail;
    }

    long size = ftell(fp);
    if(size == -1)
    {
        perror("ftell");
        goto fail;
    }

    if(fseek(fp, 0, SEEK_SET) == -1)
    {
        perror("fseek");
        goto fail;
    }

    // eh, why not?
    errno = 0;
    unsigned short gchk = global_checksum(fp, (size_t)size);
    if(gchk == (unsigned short)-1 && errno)
    {
        perror("global checksum");
        goto fail;
    }
#ifndef NDEBUG
    printf("%hx\n", gchk);
#endif
    unsigned char gchk_arr[2] = { gchk >> 8, gchk & 0xff };

    if(fseek(fp, GLOBAL_CHECKSUM, SEEK_SET) == -1)
    {
        perror("fseek");
        goto fail;
    }
    // more sanity checking
    int c1 = fgetc(fp);
    int c2 = fgetc(fp);
    if(c1 || c2)
    {
        fputs("Global checksum already in place!\n"
              "Did you run the program twice?\n", stderr);
        goto fail;
    }
    if(fseek(fp, -2, SEEK_CUR) == -1)
    {
        perror("fseek");
        goto fail;
    }
    if(fwrite(gchk_arr, 1, sizeof(gchk_arr), fp) != sizeof(gchk_arr))
    {
        perror("fwrite");
        goto fail;
    }

    fflush(fp);
    fclose(fp);

    return EXIT_SUCCESS;
fail:
    if(fp)
        fclose(fp);
    return EXIT_FAILURE;
}

What I'm looking for:

• Is there any way to simplify this? It seems that I read, seek, and re-read the file many times, and I repeat code a lot.
• Are my variable, structure, and function names self-explanatory? Do the comments help improve the readability of the program?
• Are there any edge-cases that I might've missed when testing this?
• Any other general advice.

Answer 1

Wrong compare

fgetc() returns an int with the value of EOF or something in the unsigned char range. Although EOF is commonly -1, it is not specified as so.

    int c = fgetc(fp);
    // if(c == -1)
    if(c == EOF)

Useless cast

The 2nd cast is not needed as c is in the unsigned char range so there is no value change. Before the ~ is applied, the (unsigned char)c is converted to int. No value nor type change --> cast not needed.

// result += (unsigned char)~(unsigned char)c;
result += (unsigned char)~c;

The first cast not needed either there. result += (unsigned char)~c; is same as result = result + (unsigned char)~c;. Both result and (unsigned char)~c are promoted to int before the addition. The cast in (unsigned char)~c does not affect the end result.

A cast is useful just before the assignment to quiet int to unsigned char warnings. Suggest the following:

// result += (unsigned char)~c;
result = (unsigned char) (result + ~c);

Clarity

Alternative that, IMO, is more clear.

// hchk == (unsigned char)-1
hchk == UCHAR_MAX

Strange format specifier choice

Unclear why code uses "%hx". Usually that is for unsigned short.

 unsigned char hchk;
 ...
// printf("%hx\n", hchk);
printf("%hhx\n", hchk);
// or
printf("%x\n", hchk);  // the hh is not truly needed, but it does add some clarity

Wrong error test

"on failure, the fgetpos function returns nonzero"

// if(fgetpos(fp, &pos) == -1)
if(fgetpos(fp, &pos))

Note: good use of fsetpos(), fgetpos(), versus fseek(), ftell(). Unclear why code uses fseek() elsewhere.

Performance

header_checksum() calls fgetc() to perform a checksum. There is non-trivial overhead per call. Consider re-write with a block of memory, say 256 or 4096, and fread().

The back and forth of reading Global checksum looks easy to do in one pass.

main() has too many details

I'd recommend making more helper functions.

Minor

(size_t)size relies on SIZE_MAX >= LONG_MAX. Common, but not certain. File sizes are not limited to SIZE_MAX.

Answer 2

Error printing

You do the right thing in some cases:

if(fseek(fp, -2, SEEK_CUR) == -1)
    perror("fseek");

but not others:

if(fgetpos(fp, &pos) == -1)
    return -1;

Also, that particular check does not adhere to the specification, which says:

Upon successful completion, fgetpos() shall return 0; otherwise, it shall return a non-zero value and set errno to indicate the error.

C89/C99

We've been over this a little bit in the comments, but unless there is a specific target you have in mind that requires C89, it's best to go with something more modern. I generally use C18 but C99 is also a safe bet.

Among other things, this will buy you the ability to declare and initialize variables much closer to where you actually use them in the code, something that I find helps with legibility and maintainability.

Enum offsets

This isn't a critique, but a compliment: I had forgotten (or maybe never knew?) that enum values can be computed against each other, like

MISC = ENTRY + 0x34,

That's really cool. Keep doing that.

Double-cast

(unsigned char)~(unsigned char)c;

The rightmost cast is not necessary. Whereas inversion does change the type of a term to int (TIL), it is safe to do the inversion on the character directly, and then cast it after.

Gotos

Sometimes I find that there's actually a valid application of goto; I have a few toes outside of the never-goto camp. But I don't think that's the case here. Your use of goto can be easily avoided by factoring out a function that does early-return on failure.

Answer 3

Here are some things that may help you improve your code.

Fix the bug

The program starts reading the header as though the file pointer were already pointing to location 0x100. That's an error because in all of the Gameboy files I've ever seen, the first 0x100 bytes are present and needed for calculating the checksum.

Use a struct where appropriate

This code could be made much simpler just by using a struct to represent the header. I'd use this:

struct CartridgeHeader {
    uint8_t filler[0x100];
    uint8_t entry[4];
    uint8_t logo[0x30];
    uint8_t title[0x10];
    uint8_t licensee[2];
    uint8_t SGB;
    uint8_t cart_type;
    uint8_t ROM_size;
    uint8_t RAM_size;
    uint8_t dest_code;
    uint8_t old_licensee_code;
    uint8_t mask_ROM_version;
    uint8_t header_checksum;
    uint8_t checksum_hi;
    uint8_t checksum_lo;
};

We can ignore most of these for the purposes of this program, but it only takes a minute to create the whole thing and parts of it might be useful for other purposes.

As noted in some of the comments, this assumes that the structure is not padded and that is not guaranteed by the standard. Many compilers include something like #pragma pack. If yours does, use it. Also, if your compiler supports C11, add this line to assure (at compile time) that the struct is what it needs to be:

static_assert(sizeof(struct CartridgeHeader) == 0x150, 
         "Code relies on struct having no padding");

Be efficient in file I/O

Instead of jumping around back and forth in the file, I'd suggest that a much cleaner approach would be to simply read the file once and then make a single write to update the file if needed.

Understand the header specification

The header specification says that the offset usually contains "NOP; JP 0150h" but not always. For that reason, it's not technically correct to check for those specific instructions there. The only thing the Gameboy checks for is the logo portion.

Avoid goto fail

While it may seem appealing, the goto fail as a technique is hazardous especially, as in this program, if you don't always use {} with if and for. It's difficult to make sure it is done correctly and easy to make a catastrophic error that makes international news as with Apple's infamous goto fail error. That's not something you want to be known for!

Avoid #define if you can

The problem with using a #define for data is that there is no type and therefore no type checking. Instead, you can better accomplish what you need with something like this:

static const uint8_t logo[] = {
/* Nintendo logo */ \
    0xce,0xed,0x66,0x66,0xcc,0x0d,0x00,0x0b,0x03,0x73,0x00,0x83,0x00,0x0c,0x00,0x0d, 
    0x00,0x08,0x11,0x1f,0x88,0x89,0x00,0x0e,0xdc,0xcc,0x6e,0xe6,0xdd,0xdd,0xd9,0x99,
    0xbb,0xbb,0x67,0x63,0x6e,0x0e,0xec,0xcc,0xdd,0xdc,0x99,0x9f,0xbb,0xb9,0x33,0x3e
};

Separate I/O from calculations where practical

If, as suggested above, we already have a struct, it would make sense to do the calculations on it in memory rather than as the values are being read. Here's one way to implement such a function:

static uint8_t cart_header_checksum(const struct CartridgeHeader *ch) {
    uint8_t sum = 0;
    for (uint8_t *ptr = (uint8_t *)&ch->title; ptr != &ch->header_checksum; ++ptr) {
        sum += ~*ptr;
    }
    return sum;
}

Think about more informative error return values

Most modern operating systems employ the use of an error value that can be returned from main. I'd suggest that instead of just pass/fail it might be useful if the program returned an error code suggesting what the problem was. It might look like this as an enum:

enum error_code { ERROR_NONE, ERROR_READ, ERROR_LOGO, ERROR_WRITE };

Think of the user

Rather than exiting the program with an error, I think it would be more useful to a user if the program simply told me that the checksums were correct already (if they are). If they're not, one might also want to know what values were originally and what the corrected values are. There's no need for the values to be required to be zero.

Putting it all together

Here's an alternative version that uses all of these ideas:

#include <assert.h>
#include <errno.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

struct CartridgeHeader {
    uint8_t filler[0x100];
    uint8_t entry[4];
    uint8_t logo[0x30];
    uint8_t title[0x10];
    uint8_t licensee[2];
    uint8_t SGB;
    uint8_t cart_type;
    uint8_t ROM_size;
    uint8_t RAM_size;
    uint8_t dest_code;
    uint8_t old_licensee_code;
    uint8_t mask_ROM_version;
    uint8_t header_checksum;
    uint8_t checksum_hi;
    uint8_t checksum_lo;
};

static_assert(sizeof(struct CartridgeHeader) == 0x150, "Code relies on struct having no padding");


static uint8_t cart_header_checksum(const struct CartridgeHeader *ch) {
    uint8_t sum = 0;
    for (uint8_t *ptr = (uint8_t *)&ch->title; ptr != &ch->header_checksum; ++ptr) {
        sum += ~*ptr;
    }
    return sum;
}

static bool cart_check_logo(const struct CartridgeHeader *ch) {
    static const uint8_t logo[] = {
    /* Nintendo logo */ \
        0xce,0xed,0x66,0x66,0xcc,0x0d,0x00,0x0b,0x03,0x73,0x00,0x83,0x00,0x0c,0x00,0x0d, 
        0x00,0x08,0x11,0x1f,0x88,0x89,0x00,0x0e,0xdc,0xcc,0x6e,0xe6,0xdd,0xdd,0xd9,0x99,
        0xbb,0xbb,0x67,0x63,0x6e,0x0e,0xec,0xcc,0xdd,0xdc,0x99,0x9f,0xbb,0xb9,0x33,0x3e
    };
    return memcmp(&ch->logo, logo, sizeof(logo)) == 0;
}

enum error_code { ERROR_NONE, ERROR_READ, ERROR_LOGO, ERROR_WRITE };

int main(const int argc, const char *const *const argv)
{
    if(argc != 2)
    {
        printf("Usage: %s gb-file\n", argv[0]);
        return ERROR_READ;
    }

    FILE *const fp = fopen(argv[1], "r+b");
    if(fp == NULL)
    {
        perror(argv[1]);
        return ERROR_READ;
    }

    struct CartridgeHeader header;
    if (fread(&header, 1, sizeof(header), fp) != sizeof(header)) {
        puts("Short file: header read failed");
        fclose(fp);
        return ERROR_READ;
    }
    if (!cart_check_logo(&header)) {
        puts("Logo verification failed; is this a valid file?");
        fclose(fp);
        return ERROR_LOGO;
    }

    // calculate header checksum
    uint8_t mysum = cart_header_checksum(&header);

    // calculate global checksum
    uint16_t global_sum = mysum;
    // first over part we alredy read
    for (uint8_t *ptr = (uint8_t *)&header; ptr != &header.header_checksum; ++ptr) {
        global_sum += *ptr;
    }
    // then continue with rest of file
    for (int ch = fgetc(fp); ch != EOF; ch = fgetc(fp)) {
        global_sum += ch;
    }

    if (mysum == header.header_checksum && global_sum == header.checksum_hi * 256 + header.checksum_lo) {
        puts("Cartridge already has valid checksums: nothing to do");
    } else {
        printf("calculated header checksum = %2.2x\n", mysum);
        printf("file header checksum = %2.2x\n", header.header_checksum);
        printf("calculated global sum = %4.4x\n", global_sum);
        printf("file global sum = %2.2x%2.2x\n", header.checksum_hi, header.checksum_lo);
        puts("Updating checksums");
        header.header_checksum = mysum;
        header.checksum_hi = global_sum >> 8;
        header.checksum_lo = global_sum && 0xff;
        if (fseek(fp, 0L, SEEK_SET) || fwrite(&header, 1, sizeof(header), fp) != sizeof(header)) {
            perror("Unable to write to file");
            return ERROR_WRITE;
        }
    }
    fclose(fp);
    return ERROR_NONE;
}

A few more notes

The code above reads most of the file using fgetc character at a time. While this may seem slow, modern operating systems typically use buffering and so this is not as slow as it might first seem. Another note is the code above does not attempt to distinguish between EOF and an actual file read error. This might happen, if, for example, the file is on removeable media and gets ejected during the process. This would lead to a failure of the fseek which is the next operation and so while the error message might be a bit misleading, it seemed to me not worth the bother to do anything differently. Such error checking could be added with a call to ferror if desired.

Answer 4

In addition to remarks by previous reviews:

• This is wrong:

  int main(const int argc, const char *const *const argv)
  

  The form of main() is for the compiler to decide, not the programmer. This form is not at all compatible with standard C int main(int argc, char *argv[]). So unless your compiler docs specifically tell you that your custom form of main() is ok, you are invoking undefined behavior.

• Regarding the #define HEADER_DATA, you have a subtle but severe bug, namely that each line introduces a null terminator \x00 since the data is string literals. This may screw up all CRC calculations and memcmp calls, if you don't take it in account. Wiser to roll with static const uint8_t as advised in another review.

  If you can compile as standard C, use lots of _Static_assert. For example your enum could end with a dummy entry END and that one should be the same as the size of the data, or otherwise your constants are corrupt somewhere.

• for( result = 0; nbytes--; ) should be for(result = 0; nbytes>0; nbytes--) or you will get very strange results when passing nbytes == 0 to the function.

• The result += (unsigned char)~(unsigned char)c; is code smell and the other reviews didn't get this quite right. This is what actually happens:

  • (unsigned char)c you explicitly convert from int to unsigned char.
  • ~ the compiler spots this operator and immediately and silently integer promotes back to int.
  • The result of ~ is of type int and very likely a negative number.
  • Casting to unsigned char again means that you parse out one byte from this negative number in an implementation-defined way. In practice, this will probably work just fine on most systems.
  • result += op is equivalent to result = result + op except result is only evaluated once (which doesn't matter here). Since both operands of + are small integer types, the result of + is int, but it can't be negative. You then lvalue convert this temporary int back into unsigned char upon assignment.

  Summary: way too much implicit crap going on here! C is dangerously subtle, particularly when it comes to the ~ << >> operators. There is no bug here, but this code is brittle. For rugged code with a minimum of implicit conversions, I would change this to:

  result += ~(unsigned int)c; or if you prefer result += ~(uint32_t)c;.

  This contains 2 silent promotions, result up to unsigned int and then the result of that back to unsigned char. All operands remain unsigned types, so it is harmless. More importantly, this should be fail-safe on traditional simple 8 bit checksums that rely on 8 bit unsigned wrap-around.