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

[Victor Julien]

Hi Eric, some comments:

Can you add function documentation in the doxygen format?

> +inline int CopyDataToPacket(Packet *p, uint8_t *pktdata, int pktlen)

I like the function name to start with Packet, just like
PacketPseudoPktSetup for example.

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

> +            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?

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

Cheers,
Victor

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