static const uint8_t inside function changes value

Question

I am writing a small analysis tool using libpcap that sniffs traffic on an ethernet device and performs some sort of analysis on the received packets. In order to do so, I have the obvious libpcap loop:

void packet_loop(u_char *args, const struct pcap_pkthdr *header,
        const u_char *packetdata) {
    int size = (int)header->len;

    //Before we map the buffer to the ethhdr struct,
    //we check if the size fits
    if (ETHER_HDR_LEN > size)
        return;

    const struct ethhdr *ethh = (const struct ethhdr *)(packetdata);

    //If this protocol is IPv4 and the packet size is bigger than
    //ETH hdr size
    if (ETHERTYPE_IP == ntohs(ethh->h_proto)) {

        //Before we map the buffer to the iph struct, 
        //we check if the size fits
        if (ETHER_HDR_LEN + (int)sizeof(struct iphdr) > size)
            return;

        const struct iphdr *iph = (const struct iphdr*)
            (packetdata + sizeof(struct ethhdr));

        //If this protocol isn't UDP and the header length
        //isn't 5 (20bytes)
        if (IPPROTO_UDP != iph->protocol && 5 != iph->ihl)
            return;

        //eval_udp(packetdata, size);
        const struct udphdr *udph = (const struct udphdr*)
            (packetdata + sizeof(struct ethhdr) + 
             sizeof(struct iphdr));

        if (DATA_SRCPORT == ntohs(udph->uh_sport) &&
            DATA_DESTPORT == ntohs(udph->uh_dport)) {
            analyse_data(packetdata);
        }
    }
}

that calls the follwoing code snipped on receival of a specific packet type. As you can see, I am using a static variable to keep track of the previous packet, in order to compare two.

void analyse_data(const uint8_t *packet)
{
    if (!packet)
        return;

    static const uint8_t *basepacket;

    //If there was no packet to base our analysis on, we will wait for one
    if (!basepacket) {
        basepacket = packet;
        return;
    }

    const struct dataheader *basedh = (const struct dataheader *)
          (__OFFSETSHERE__ + basepacket);
    const struct dataheader *dh = (const struct dataheader *)
          (__OFFSETSHERE__ + packet);

    printf("%d -> %d\n", ntohs(basedh->sequenceid),
                         ntohs(dh->sequenceid));

    basepacket = packet;
    return;
}

struct dataheader is a regular struct, just like etthdr. I would expect a constant printout like:

0 -> 1
1 -> 2
2 -> 3

Unfortunately, I get a different printout, which is mostly right. But around every 20th-40th packet, I see the following behavior (example):

12->13
13->14
0->15
15->16
...

It is maybe interesting to note that this does NOT occcur, when I receive only packets of the specific type I look after (8-10 Mbit/s). Nevertheless, as soon as I use my tool in the "regular" network environment (around 100Mbit/s), I get this behavior. I checked my if statement, that filters the packet it works flawlessly (checking UDP source and destination ports). Wireshark also shows me that there is not a single packet on those ports that is not of that specific type.

Most likely due to some undefined behaviour in the code that you haven't pasted up. — Bathsheba, Dec 08 '17 at 10:07
I do not believe this. I debugged that part thoroughly. And it is really just simple dissecting and printing out. Furthermore (referring to the example), packet's sequence id was 14 and checked to 0 when basepacket was set to packet. — TacoVox, Dec 08 '17 at 10:09
Well, something is wrong and it ain't due to the simple compares and assignments you make above. — Bathsheba, Dec 08 '17 at 10:09
So you do not believe that basepacket can change its value? / The data get lost when I set basepacket = packet — TacoVox, Dec 08 '17 at 10:12
@TacoVox: The part you have not pasted up includes the entire rest of the program, not just the code in this function. Some bug could be overwriting `basepacket`. You do not even show the `printf` statements, so we do not see where these numbers you are printing come from. (Some bug may be changing a packet‘s sequence number.) We do not see what `nextpacket` is. We do not see if this program is multithreaded. You have omitted important things. — Eric Postpischil, Dec 08 '17 at 10:36
I updated the text accordingly. Hope it is more clear now :) I actually made a mistake while pasting ;) but here we go. that is the code. The method call is in the same thread as the pcap loop. — TacoVox, Dec 08 '17 at 11:10
Still not a complete example. You're passing a pointer into `packet_loop()` that you save into a `static` value and then dereference in a later iteration. Where does the `packetdata` pointer refer to? What happens to that memory in between calls to `packet_loop()`. You've left all that out. — Andrew Henle, Dec 08 '17 at 11:26
Eric: Nothing happens to the packet after packet_loop returns. Nothing should happen. I just want to make the analysis in that function. :) Just one thread runs the pcap loop. @AndrewHenle I actually did not leave that out. That is the regular libpcap loop signature and i just pass along that packet. I do not do any mem operations afterwards. — TacoVox, Dec 08 '17 at 11:36
@TacoVox: It cannot be true that nothing happens to the packet after `packet_loop` returns. Either you free its storage at some time or reuse it or your process eventually runs out of memory. — Eric Postpischil, Dec 08 '17 at 11:54
Let’s recap. The problem was in code you did not originally include in the problem and said you thoroughly debugged, the problem is affected by code you still have not included, something does happen to the packet afterward although you said nothing happens, and the title is false. Please tell us you have learned something about asking questions and diagnosing problems. — Eric Postpischil, Dec 08 '17 at 13:16
Unfortunately, I have to strongly disagree. Please check the great answer provided by nos. Here you will see that it had nothing to do with the code that I omitted in the first place. It has something to do with how libpcap handles the incoming packets after the loop body returns. And I did not free the packets and did not run out of memory, as libpcap was apparently doing it for me all the time. — TacoVox, Dec 08 '17 at 15:12
And what do you want me to include? The two liner that start the pcap loop? That makes no sense to me. I have absolutely no influence on how libpcap is designed... And sometimes it would be nice to have more friendly people here. I did not harm anyone of you here and I am just facing an unfriendly attitude. — TacoVox, Dec 08 '17 at 15:14
The code you did not include modifies a packet after `packet_loop` returns, likely by reusing it (either for another packet or by freeing its memory, which was subsequently reallocated). Thus, `basepacket` is left pointing at storage that no longer contains the original packet. So, when you print `basedh->sequenceid`, it contains 0 instead of the value it had when it was `dh_sequenceid` in a previous call to `analyse_data`. And, in fact, the `static const uint8_t *` that is `basepacket` did not change—the pointer stayed the same (so the title is false). Rather, the pointed-to storage changed. — Eric Postpischil, Dec 08 '17 at 15:26
So, yes, you originally left out the `printf` that would let us see specifically what you were printing, and you left out the code that manages packets which would let us see that packets were being reused or freed or modified, and you wrote incorrectly in the title that a `static` object changed when in fact it did not change but rather the storage it pointed to did. As for whether people are friendly or not, they have simply stated facts: There was missing code and information that were necessary to diagnose the problem. — Eric Postpischil, Dec 08 '17 at 15:30

score 2 · Accepted Answer · answered Dec 08 '17 at 11:35

libpcap controls the packet data it passes in to your packet_loop. Once packet_loop returns, you have no guarantee what the pointers for the packet data point to - libpcap could throw the packet away, or it could reuse the same space for a new packet.

This means if you want to compare 2 packets, you must make a copy of the 1. packet - you cannot save the pointer from one call to packet_loop and expect that pointer to be valid and point to the same packet in future calls to packet_loop. So your code could be changed to e.g.

void analyse_data(const uint8_t *packet, int size )
{
    if (!packet)
        return;

    static const uint8_t basepacket[1024*64];
    static int has_basepacket;

    //If there was no packet to base our analysis on, we will wait for one
    if (!has_basepacket){
        if (size < sizeof basepacket) {
            memcpy(basepacket, packet, size);
            has_basepacket = 1;
         }
        return;
    }
    ...

Also, make sure your verify the sizes everywhere. Just because the ethernet type says it is an IPv4 packet, doesn't mean you can trust it to contain a full IP packet. Just because the IP header says it is 20 bytes, doesn't mean you can trust it to contain a full IP packet, and so on for all the layers you attempt to decode.

Thanks for this clarification! That was what I was looking for :) — TacoVox, Dec 08 '17 at 11:43

static const uint8_t inside function changes value

1 Answers1