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

[Eric Leblond]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Le 17/11/2010 09:02, Victor Julien a écrit :
> 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.

Ok, I will fix this.

> 
>>      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.

My idea was the following: suricata has to deal with weird packet and
some frag may change the size of the global packet. I thus take the
decision to allocate full length to avoid doing some reallocation in
case of such behaviour. I may be wrong and in this case, I fully agree
to use the provided packet length.

> 
>> +            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) {
...
>> +            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 :)

These modifications have been made for the same reason. If a frag
announce a new length, we may need to move the data from static array to
the allocated part. By the way, it make me think that almost the same
should be done to PacketCopyData. I've wrongly assume that it will only
be called from the source package.

> 
> Looking good otherwise! Thanks Eric.
> 
> Cheers,
> Victor
> 
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iD8DBQFM5FQwnxA7CdMWjzIRAp8TAJ9zhWb+dsfn6Yoi7ufvSn5U3ycg5QCcDOh2
hKGB3nRgiTgSKlfLTwAVBnk=
=p2Eh
-----END PGP SIGNATURE-----