Custom checksum algorithm

Question

A while back, I reverse-engineered a checksum algorithm from an MMO used to check the validity of an item that's linked to chat (similar to WoW). The idea is that if the checksum is invalid then the game client would ignore the link when clicked. Otherwise clicking on the item link in in-game chat would display the stat and attrib for that item.

ushort16 CreateChecksum(const string &itemlink)
{
    stringstream parseitemlink(itemlink);
    uint32 hexform[ITEMLINKGROUPCOUNT] = {0};
    uint32 hexsum = 0;

    //Parse itemLink string into hexform array
    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
        parseitemlink >> hex >> hexform[i];

    //sum all the itemlink group together
    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
        hexsum += hexform[i];

    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
    {
        uint32 ebx = hexform[i], edi = ebx * i;

        //if loop iteration is odd store MSB 2-bytes.
        //Otherwise, store working hexgroup as is untouched
        if (i & 0x01)
            ebx = hexform[i] >> 16; // aka same as dividing hexform[i] by 65,536

        // (hexform[i] / 65,536) + (hexform[i] * i) + hexsum -- for odd groups
        //  hexform[i] + (hexform[i] * i) + hexsum           -- for even groups
        ebx += edi + hexsum;
        hexsum = ebx ^ hexform[i];
    }

    for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
    {
        // the more familiar high-level form would be
        // hexform[i]^2 + hexform[i] * hexsum
        uint32 ecx = (hexform[i] + 1) * hexsum,
               eax = ecx * hexform[i];

        eax >>= 16;
        eax += ecx;
        hexsum = eax ^ hexform[i];
    }

    //return the lower 2-bytes of hexsum
    //as the final checksum
    return hexsum & 0xFFFF;
}//CreateChecksum

The format of the itemlink is comprised of a group of hexadecimal separated with a space in string format. It's passed into main() as an argument when the program is run.

Here's what an itemlink's hex string might look like:

const string EXAMPLELINK = "36c6a 0 3f000a54 d0f1 0 0 0 0 0 0 20d0";

Are there any code smells or readability issues in this code segment? Can any part(s) of it be improved?

Answer 1

If you are using the standard library classes of the same name, I would give the following names the correct namespace qualifier: std::string, std::stringstream, std::hex.

In C++, this works just as well, IMHO it's mildy more idiomatic.

uint32 hexform[ITEMLINKGROUPCOUNT] = {};

ebx, edi, ecx, eax are not good variable names, if you can give them more meaningful names, then do.

    uint32 ecx = (hexform[i] + 1) * hexsum,
           eax = ecx * hexform[i];

Personally, I think this is clearer:

    uint32 ecx = (hexform[i] + 1) * hexsum;
    uint32 eax = ecx * hexform[i];

The comment is really bad because it talks about hexform[i]^2 + hexform[i] * hexsum whereas ecx gets the value hexform[i] * hexsum + hexsum and eax gets the value hexform[i]^2 * hexsum + hexform[i] * hexsum. I think the comment needs a pair of parentheses if the code is doing what you meant.

To be robust, you should check whether the parse worked.

parseitemlink >> hex >> hexform[i];

You can trivially combine the first two for loops as well.

Answer 2

• Function and variable names in C++ are commonly camelCase or snake_case, while uppercase names are for user-defined types.

  If ITEMLINKGROUPCOUNT is a variable or a constant, then it should also follow camelCase or snake_case. All-caps naming is commonly used for macros, and the different words should be separated with underscores.

• uint32 is not standard C++ nor is it even C (which is uint32_t). Use std::uint32_t from <cstdint>. More info about that here and here.

• Instead of this sum loop:

  for (int i = 0; i < ITEMLINKGROUPCOUNT; ++i)
      hexsum += hexform[i];
  

  use std::accumulate as a more clean and C++-like alternative:

  uint32 hexsum = std::accumulate(hexform, hexform+ITEMLINKGROUPCOUNT, 0);
  

  On another note, hexsum should be initialized here as this is where it's first used. Always keep variables as close in scope as possible.

Answer 3

It seems to me your code currently displays the fact that it was reverse engineered from assembly language a little more than I'd like.

I'd try to rewrite it in a form closer to how you'd normally write C++ instead of basically just translating assembly language into C++ syntax, but retaining most of the assembly language flavor (up to, and still including, using the register names for your variables).

For example, reading the input string and converting from strings to a vector of uint32 could be more like this:

class hex_word {
    uint32 val;
public:
    friend std::istream& operator>>(std::istream &is, hex_word &h) { 
        return is >> hex >> h;
    }
    operator uint32() { return val; }
};

std::vector<uint32> hexform{std::istream_iterator<hex_word>(parseitemlink),
                            std::istream_iterator<hex_word>()};

The to add those up, you could use std::accumulate:

uint32 hexsum = std::accumulate(hexform, hexform+ITEMLINKGROUPCOUNT, 0);

Skipping ahead a little, your last loop could use std::accumulate:

struct f {
    uint32 operator()(uint32 accumulator, uint32 val) {
        uint32 c = (val+1)*accumulator; 
        uint32 a = c * val;
        return ((a>>16)+c) ^ accumulator;
    }
};

return std::accumulate(hexform.begin(), hexform.end(), hexsum, f) * 0xffff;

I'm not sure this leads to any startling new insights or dramatic simplification of the code, but it still strikes me as rather easier to understand than with all the code mashed together.

Answer 4

The only things I can see having problems are your acronym variable names i.e. (eax, ecx, ebx, and edi) don't explain what the variables are storing clearly for someone not experienced with doing checksums.

The other thing I can see for readability issues is your all lowercase variable names which are descriptive (ie parseitemlink) should be camelCased so I can tell the different words (ie parseItemLink). That's the only thing I can think of readability wise.