Search code examples
cstructshort

Unsigned short changes value when assigning to struct field


I have a function to parse 20 bytes long IP header buffer:

void parseIp(struct ipHeader *ip, const void *buffer)
{
    uint8_t* b = buffer;
   // memcpy(b,buffer,20);
    ip->version = (b[0] & 0xf0) >> 4;
    ip->ihl = (b[0] & 0x0f);
    ip->dscp = (b[1] & 0xfC)>>2;
    ip->ecn = (b[1] & 0x3);

    unsigned short l = (b[2] << 8) | b[3];

    printf("%d\n",l);
    ip->length = l;
    ip->identification = (b[4] << 0xFF) | b[5];
}

struct ipHeader:

struct ipHeader {
    int version;
    int ihl;
    int dscp;
    int ecn;
    unsigned short length;
    unsigned short identification;
    int flags;
    int fragment_offset;
    int time_to_live;
    int protocol;
    unsigned short header_checksum;
    unsigned char source_ip[4];
    unsigned char destination_ip[4];
};

Now the code prints l as 467, which is correct, but as this l is assigned to the struct field length it changes to 54017. I don't understand at all what is going on. I added the variable l to make sure no overflowing or type conversion errors could happen, but still it changes.

This is part of school work, so I can't change the struct.

EDIT full code:

#include <stdio.h>
#include <arpa/inet.h>
#include "ipheader.h"


/* Parses the given buffer into an IP header structure.
 * 
 * Parameters:
 * ip: pointer to the IP header structure that will be filled based
 *      on the data in the buffer
 * buffer: buffer of 20 bytes that contain the IP header. */
    void parseIp(struct ipHeader *ip, const void *buffer)
    {
        uint8_t* b = buffer;
       // memcpy(b,buffer,20);
        ip->version = (b[0] & 0xf0) >> 4;
        ip->ihl = (b[0] & 0x0f);
        ip->dscp = (b[1] & 0xfC)>>2;
        ip->ecn = (b[1] & 0x3);

        unsigned short l = (b[2] << 8) | b[3];

        printf("%d\n",l);
        ip->length = l;
        ip->identification = (b[4] << 8) | b[5];
    }


/* Builds a 20-byte byte stream based on the given IP header structure
 * 
 * Parameters:
 * buffer: pointer to the 20-byte buffer to which the header is constructed
 * ip: IP header structure that will be packed to the buffer */
void sendIp(void *buffer, const struct ipHeader *ip)
{
}


/* Prints the given IP header structure */
void printIp(const struct ipHeader *ip)
{
    /* Note: ntohs below is for converting numbers from network byte order
     to host byte order. You can ignore them for now
     To be discussed further in Network Programming course... */
    printf("version: %d   ihl: %d   dscp: %d   ecn: %d\n",
            ip->version, ip->ihl, ip->dscp, ip->ecn);
    printf("length: %d   id: %d   flags: %d   offset: %d\n",
            ntohs(ip->length), ntohs(ip->identification), ip->flags, ip->fragment_offset);
    printf("time to live: %d   protocol: %d   checksum: 0x%04x\n",
            ip->time_to_live, ip->protocol, ntohs(ip->header_checksum));
    printf("source ip: %d.%d.%d.%d\n", ip->source_ip[0], ip->source_ip[1],
            ip->source_ip[2], ip->source_ip[3]);
    printf("destination ip: %d.%d.%d.%d\n", ip->destination_ip[0],
            ip->destination_ip[1],
            ip->destination_ip[2], ip->destination_ip[3]);    
}

/* Shows hexdump of given data buffer */
void hexdump(const void *buffer, unsigned int length)
{
    const unsigned char *cbuf = buffer;
    unsigned int i;
    for (i = 0; i < length; ) {
        printf("%02x ", cbuf[i]);
        i++;
        if (!(i % 8))
            printf("\n");
    }
}

struct ipHeader {
    int version;
    int ihl;
    int dscp;
    int ecn;
    unsigned short length;
    unsigned short identification;
    int flags;
    int fragment_offset;
    int time_to_live;
    int protocol;
    unsigned short header_checksum;
    unsigned char source_ip[4];
    unsigned char destination_ip[4];
};

void parseIp(struct ipHeader *ip, const void *buffer);
void sendIp(void *buffer, const struct ipHeader *ip);

void printIp(const struct ipHeader *ip);
void hexdump(const void *buffer, unsigned int length);


#include <arpa/inet.h>
#include "ipheader.h"

int main()
{
    /* Feel free to modify this function to test different things */

    unsigned char bytes[] = {
        0x45, 0x00, 0x01, 0xd3, 0xda, 0x8d, 0x40, 0x00,
        0x40, 0x06, 0x8c, 0xd5, 0xc0, 0xa8, 0x01, 0x46,
        0x6c, 0xa0, 0xa3, 0x33 };

    struct ipHeader ip;

    parseIp(&ip, bytes);
    printIp(&ip);

    struct ipHeader ipfields = {
        4, // version
        28, // ihl
        4, // dscp
        0, // ecn
        htons(1500), // length
        htons(1234), // id
        1, // flags
        1024, // offset
        15, // time_to_live
        33, // protocol
        htons(0x1234), // checksum (invalid)
        {1, 2, 3, 4}, // source IP
        {5, 6, 7, 8} // destination IP
    };
    unsigned char sendbuf[20];

    sendIp(sendbuf, &ipfields);
    hexdump(sendbuf, sizeof(sendbuf));
}

Solution

  • For the given input:

    unsigned char bytes[] = { 0x45, 0x00, 0x01, 0xd3, 0xda, 
    

    then the code:

    unsigned short l = (b[2] << 8) | b[3];
    

    produces l with a value of 467.

    You say in your question, "as this l is assigned to the struct field length it changes to 54017. ". However this is not true. If you add a line immediately after your existing ip->length = l;:

    printf("%d\n", ip->length);
    

    you will still see 467.


    I guess the problem you refer to is that your printIp function prints 54017.This is because that function does not print ip->length. It prints ntohs(ip->length). The ntohs macro changes the value from 567 to 54017.

    To fix this, change the printIp function to print ip->length, and not ntohs(ip->length) .

    Remove the other ntohs calls from that function too, and remove htons from your definition of ipfields. The integers are supposed to be stored in host order (i.e. native order) inside the struct ipHeader, and stored in network order (i.e. big-endian) when in the unsigned char buffer.


    Portability Note 1: Technically you should use %hu as format specifier in both of those printf statements, because the argument type was unsigned short.

    Portability Note 2: l == 467 regardless of int size, contrary to what has been suggested in some of the comments/answers so far. But to support values of b[2] greater than 0x7F when running on a system with 16-bit int, you should write ((unsigned)b[2] << 8) | b[3].

    Portability Note 3: It would be a good idea to use uint16_t instead of unsigned short because there are now systems with 32-bit unsigned short. If you do this, the printf format specifier is "%"PRI16u for which you may need #include <inttypes.h>