16-bit Checksum function in C I was playing with TCP/IP, and came across this 16 bit checksum function:

unsigned short csum(unsigned short *ptr,int nbytes) 
{
    register long sum;
    unsigned short oddbyte;
    register short answer;

    sum=0;
    while(nbytes&gt;1) {
        sum+=*ptr++;
        nbytes-=2;
    }
    if(nbytes==1) {
        oddbyte=0;
        *((u_char*)&amp;oddbyte)=*(u_char*)ptr;
        sum+=oddbyte;
    }

    sum = (sum&gt;&gt;16)+(sum &amp; 0xffff);
    sum = sum + (sum&gt;&gt;16);
    answer=(short)~sum;

    return(answer);
}


There were a few things that bothered me about it:


Not using fixed size types (though the author himself stated in the code short was expected to be 16 bits for it to work)
If the input buffer was too large (I believe larger than exactly 65535 bytes) the checksum wouldn't be correct, since the most significant 2 bytes of the checksum variable would overflow.


So I ended up rewriting it:

uint16_t checksum16(void *data, unsigned int bytes){
    uint16_t *data_pointer = (uint16_t *) data;
    uint32_t total_sum;

    while(bytes &gt; 1){
        total_sum += *data_pointer++;
        //If it overflows to the MSBs add it straight away
        if(total_sum &gt;&gt; 16){
            total_sum = (total_sum &gt;&gt; 16) + (total_sum &amp; 0x0000FFFF);
        }
        bytes -= 2; //Consumed 2 bytes
    }
    if(1 == bytes){
        //Add the last byte
        total_sum += *(((uint8_t *) data_pointer) + 1);
        //If it overflows to the MSBs add it straight away
        if(total_sum &gt;&gt; 16){
            total_sum = (total_sum &gt;&gt; 16) + (total_sum &amp; 0x0000FFFF);
        }
        bytes -= 1;
    }

    return (~((uint16_t) total_sum));
}


Since it adds the carry bit right after the sum, you can feed it with a buffer of any size with no worries of the final checksum being incorrect.

After some small tests, the results seem to be matching. Do you guys see any possible bug while using that function? Or any further improvement?

I thought maybe I could add a statement to check if the number of bytes was larger than 65535, and only add the carry on each iteration if it was, otherwise adding the whole sum of carries at the end. That would improve the performance on the cases where the size of the buffer doesn't reach that limit (true to any IP packet, since the header length field is 16 bits, limiting the packet size to 65535).
Since this checksum function is only used for TCP packets, there is no need for supporting packets larger than 65535 bytes. Rather than adding needless overflow checks, you should restrict the bytes parameter by making that a uint16_t.

Note that both the original code and your improvement only work on big endian implementations, unless some outside code byteswapped the whole TCP packet, in which case the code only works on little endian implementations. The + 1 for accessing the last byte makes the latter more probable. This + 1 was not present in the original code, so chances are high that you introduced a bug. Did you test with odd-sized packet length?

You should make the spacing of the code consistent, either by removing all spaces around binary operators (not recommended) or inserting spaces at the appropriate places. Currently it is a mixture of both.

Since you don't modify the incoming buffer, you should declare that. Make the data const void *.

You don't need to write 1 == bytes, since any decent compiler will emit a warning if you would ever accidentally use a single = here. You should write the natural bytes == 1 instead, or if you fear typos, bytes &gt; 0.

The bytes -= 1 is unnecessary and can be left out.