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

[Eric Leblond]

Hi

Le 16 nov. 2010 à 08:51, Victor Julien <victor at inliniac.net> a écrit :

> Hi Eric, some comments:
> 
> Can you add function documentation in the doxygen format?

Ok

> 
>> +inline int CopyDataToPacket(Packet *p, uint8_t *pktdata, int pktlen)
> 
> I like the function name to start with Packet, just like
> PacketPseudoPktSetup for example.

Ok it seems a nice move.

> 
>> +{
>> +    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);
> 
> We can't assume SCMalloc will succeed here. Special traffic conditions,
> an attacker, or other processes on the IDS/IPS box may cause malloc to
> fail. We should be able to cope with that the best we can.

Oups, I ve missed that one, sorry.

> 
>> +            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;
>> +}
> 
> Can you add function documentation in the doxygen format?

Ok

> 
>> +inline int CopyDataToPacketOffset(Packet *p, int offset, uint8_t *data, int datalen)
> 
> Same remark on the function name.
> 
>> +{
>> +    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);
> 
> Check malloc.
> 
>> diff --git a/src/decode.h b/src/decode.h
>> index b911ed5..66fb319 100644
>> --- a/src/decode.h
>> +++ b/src/decode.h
>> @@ -57,6 +57,10 @@
>> 
>> #include "detect-reference.h"
>> 
>> +#define DFLT_PAYLOAD_SIZE 1500
> 
> 1514 is a better value I think, in my non-nfq testing it's what I see as
> max pkt size all the time.

Ok that was a question I forget to ask.

I send you a new version as soon as possible.

Thanks a lot for reviewing this code.

BR

Eric

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