PayloadLayer returns wrong size for payload when inherited getLayerPayloadSize is called

[maruu]

Hi,

this is my first time opening an issue on Github. I will try to provide as much information as I have.

As the title states, PayloadLayer returns wrong size for payload when inherited getLayerPayloadSize is called.

The following code can be used to reproduce the issue:

uint8_t data[] = { 0xde, 0xad, 0xbe, 0xef };
size_t dataLen = 4;

/* Create packet and fill it with payload */
pcpp::Packet packet;
pcpp::PayloadLayer* pPayloadLayer = new pcpp::PayloadLayer(data, dataLen, false);
packet.addLayer(pPayloadLayer, true);

pcpp::PayloadLayer* pPayload = packet.getLayerOfType<pcpp::PayloadLayer>();

assert(pPayload->getLayerPayloadSize() == 4) /* getLayerPayloadSize returns 0 */

The problem arises because getHeaderLen of PayloadLayer returns m_DataLen. When calling getLayerPayloadSize of base class Layer, m_DataLen is subtracted by getHeaderLen of derived layer, effectively resulting in 0 being returned.

I am currently running Windows10, but I don't think that it matters in this case.

Note: I currently cannot pull the newest master since I have made changes to the PcapPlusPlus Packet++ source code to suite my needs. If this has already been fixed in the current master, then I am sorry for filing this issue.

PS: I really enjoy the work of this community. Please keep this wonderful piece of software going.

[seladb]

@maruu thanks for reaching out and thank you for the kind words.

The getLayerPayloadSize() method name is a little confusing in this case. Let me explain:

Each protocol (layer) has it's own header and payload. For example: in IPv4 the header includes the IP addresses, IP ID, total length, flags, TTL, next protocol and checksum. Everything that comes after the header is the layer's payload. So in the IPv4 case getLayerPayloadSize() will give you the size of everything that comes after the IPv4 header.

PayloadLayer is a generic layer which is introduced when PcapPlusPlus doesn't know or doesn't have the parser for that layer. For example: SCTP is not yet implemented in PcapPlusPlus so if you try to parse an SCTP packet you'll probably see these layers:

EthLayer
IPv4Layer
PayloadLayer

This means that PcapPlusPlus recognized and parsed the Ethernet and IPv4 layers, but since it didn't recognize the SCTP layer it used PayloadLayer instead.

PayloadLayer is just a wrapper class for the collection of bytes PcapPlusPlus couldn't parse, so it doesn't really have an header or payload. The decision was that getHeaderLen() for this layer will return the size of the whole data, which supposingly means that this layer has only header and not payload, but of course this is just an arbitrary decision (which serves multiple purposes). Anyway, this is why getLayerPayloadSize() will always return 0 for this layer.

I understand the naming is a bit confusing here but basically you shouldn't use getLayerPayloadSize() for this layer. Instead you should use PayloadLayer::getPayloadLen(). This will give you the size of the PayloadLayer data.

This turned out to be a very long explanation, I'm sorry about that. I hope it's clear though.

Please let me know if you have more questions.

[maruu]

Is there a good explanation for using getHeaderLen() for retrieving the payload of a PayloadLayer?

For me, it would be semantically more correct to let PayloadLayer return 0 on getHeaderLen(). This would also solve the getLayerPayloadSize() problem, which would then return the correct size of the payload.

As a new user to PCAP++, I would expect the PayloadLayer to not have any header information, because it cannot know which header it should parse.

If this is still intended behavior from your side, you can kindly close this issue and I am sorry for raising it :-)

Thank you for your fast response. This is one of the reasons this library has been chosen for my further development. Thanks!

[seladb]

Yes, there is a specific reason for that:

The way PcapPlusPlus parses a packet is as follows (pseudo code, very high level, not 100% accurate):

packet_pointer = 0
While packet_pointer hasn't reached the end of the packet:
    Parse the next layer from where packet_pointer is
    Add the parsed layer to the list of layers in the packet
    parsed_layer_payload_pointer = pointer_to_beginning_of_layer + new_layer->getHeaderLen()
    packet_pointer = parsed_layer_payload_pointer 

Again, this is very high level but I think it shows the point: if PayloadLayer::getHeaderLen() would be 0 the packet parser will look for the next layer in an endless loop

[maruu]

Thanks a lot for this clarification. It is now clear to me why you have chosen this way for implementation.