[Oisf-devel] [PATCH 1/2] Modify Packet structure and prepare accessor.

[Victor Julien]

Eric Leblond wrote:
> @@ -107,8 +108,8 @@ Packet *PacketPseudoPktSetup(Packet *parent, uint8_t *pkt, uint16_t len, uint8_t
>  
>      /* copy packet and set lenght, proto */
>      p->tunnel_proto = proto;
> -    p->pktlen = len;
> -    memcpy(&p->pkt, pkt, len);
> +    SET_PKT_LEN(p, len);
> +    PacketCopyData(p, pkt, len);

The PacketCopyData's return code is ignored, even on cases where malloc
failed.

>      p->recursion_level = parent->recursion_level + 1;
>      p->ts.tv_sec = parent->ts.tv_sec;
>      p->ts.tv_usec = parent->ts.tv_usec;
> @@ -243,3 +244,109 @@ DecodeThreadVars *DecodeThreadVarsAlloc() {
>  
>      return dtv;
>  }
> +
> +/**
> + *  \brief Copy data to Packet payload
> + *
> + *  \param Pointer to the Packet to modify
> + *  \param Pointer to the data to copy
> + *  \param Length of the data to copy
> + */
> +inline int PacketCopyData(Packet *p, uint8_t *pktdata, int pktlen)
> +{
> +    if (pktlen > MAX_PAYLOAD_SIZE) {
> +        /* too big */
> +        return -1;
> +    }
> +    if (! p->ext_pkt) {
> +        if (pktlen < DFLT_PAYLOAD_SIZE) {
> +            memcpy(&p->pkt, pktdata, pktlen);
> +            SET_PKT_LEN(p, (size_t)pktlen);
> +        } else {
> +            /* here we need a dynamic allocation */
> +            p->ext_pkt = SCMalloc(MAX_PAYLOAD_SIZE);

Why is MAX_PAYLOAD_SIZE alloc'd here? We know what size we want, right?
And the ext_pkt is freed when the packet is returned to the packet pool,
so it's not for the next data read.

> +            if (p->ext_pkt == NULL) {
> +                SCLogError(SC_ERR_MEM_ALLOC, "SCMalloc failed: %s", strerror(errno));
> +                SET_PKT_LEN(p, 0);
> +                return -1;
> +            }
> +            memcpy(p->ext_pkt, pktdata, pktlen);
> +            SET_PKT_LEN(p, (size_t)pktlen);
> +        }
> +    } else {
> +            memcpy(p->ext_pkt, pktdata, pktlen);
> +            SET_PKT_LEN(p, (size_t)pktlen);
> +    }
> +    return 0;
> +}
> +
> +/**
> + *  \brief Copy data to Packet payload at given offset
> + *
> + *  \param Pointer to the Packet to modify
> + *  \param Offset of the copy relatively to payload of Packet
> + *  \param Pointer to the data to copy
> + *  \param Length of the data to copy
> + */
> +inline int PacketCopyDataOffset(Packet *p, int offset, uint8_t *data, int datalen)
> +{
> +    if (offset + datalen > MAX_PAYLOAD_SIZE) {
> +        /* too big */
> +        return -1;
> +    }
> +
> +    if (! p->ext_pkt) {
> +        if (offset + datalen <= DFLT_PAYLOAD_SIZE) {
> +            memcpy(&p->pkt + offset, data, datalen);
> +        } else {
> +            /* here we need a dynamic allocation. This case should rarely
> +             * occur as there is a high probability the first frag has
> +             * reveal the packet size*/
> +            p->ext_pkt = SCMalloc(MAX_PAYLOAD_SIZE);
> +            if (p->ext_pkt == NULL) {
> +                SCLogError(SC_ERR_MEM_ALLOC, "SCMalloc failed: %s", strerror(errno));
> +                SET_PKT_LEN(p, 0);
> +                return -1;
> +            }
> +            /* copy initial data */
> +            memcpy(p->ext_pkt, &p->pkt, p->pktlen);
> +            /* copy data as asked */
> +            memcpy(p->ext_pkt + offset, data, datalen);
> +            /* modify part of the struct ipv4 that were pointing to pkt array
> +             * using the fact that offset is the same*/
> +            if (p->ethh) {
> +                p->ethh = (EthernetHdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->ethh - p->pkt));
> +            }
> +            if (p->ip4h) {
> +                p->ip4h = (IPV4Hdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->ip4h - p->pkt));
> +            }
> +            if (p->ip6h) {
> +                p->ip6h = (IPV6Hdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->ip6h - p->pkt));
> +            }
> +            if (p->tcph) {
> +                p->tcph = (TCPHdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->tcph - p->pkt));
> +            }
> +            if (p->udph) {
> +                p->udph = (UDPHdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->udph - p->pkt));
> +            }
> +            if (p->icmpv4h) {
> +                p->icmpv4h = (ICMPV4Hdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->icmpv4h - p->pkt));
> +            }
> +            if (p->icmpv6h) {
> +                p->icmpv6h = (ICMPV6Hdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->icmpv6h - p->pkt));
> +            }
> +            if (p->ppph) {
> +                p->ppph = (PPPHdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->ppph - p->pkt));
> +            }
> +            if (p->greh) {
> +                p->greh = (GREHdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->greh - p->pkt));
> +            }
> +            if (p->vlanh) {
> +                p->vlanh = (VLANHdr *)((uint8_t *)p->ext_pkt + ((uint8_t *)p->vlanh - p->pkt));
> +            }

Whats the purpose of having this here? The only place I see this
function in use is in the defrag engine. There the decoding will run
after this function has run. If any of the unittests depend on these
pointers being set here the tests are broken and need fixing :)

Looking good otherwise! Thanks Eric.

Cheers,
Victor


-- 
---------------------------------------------
Victor Julien
http://www.inliniac.net/
PGP: http://www.inliniac.net/victorjulien.asc
---------------------------------------------