[Oisf-devel] Packet Array in 1.0.x

[Victor Julien]

Joel Ebrahimi wrote:
> I think the problem with this safety catch and the initialization is
> that when Suricata starts my max_pending_packets is 0, so this will
> initialize pcap_max_read_packets to 0. 

I wonder how it can be 0 though. In suricata.c:main() we have:

    /* Pull the max pending packets from the config, if not found fall
     * back on a sane default. */
    if (ConfGetInt("max-pending-packets", &max_pending_packets) != 1)
        max_pending_packets = DEFAULT_MAX_PENDING_PACKETS;
    SCLogDebug("Max pending packets set to %"PRIiMAX, max_pending_packets);

This code is executed long before the threads are initialized. So either
max_pending_packets is set to DEFAULT_MAX_PENDING_PACKETS or it is set
to whatever value you have for max-pending-packets in your config.

Could you check to see if this part works for you?

> So this 0 value is then passed in as the cnt variable to pcap_distpatch.
> This 0 causes pcap_distpatch to grab all the packets in the buffer.
> Where the buffer can be any number and in my cases I see it vary between
> 12000 and 15000, but any number beyond 256 causes the application to
> segfault. 

Understood. If it is 0 that makes sense.

> In looking at the initialization function, if I add an extra check for
> pcap_max_read_packets at 0 and change it, this actually fixes the
> problem and Suricata does not segfaults anymore. Basically I changed
> this check:
> 
>     pcap_max_read_packets = (PCAP_FILE_MAX_PKTS < max_pending_packets) ?
>         PCAP_FILE_MAX_PKTS : max_pending_packets;
> 
> to:
> 
>     pcap_max_read_packets = (PCAP_FILE_MAX_PKTS < max_pending_packets) ?
>         PCAP_FILE_MAX_PKTS : max_pending_packets;
>     if(pcap_max_read_packets == 0)
>         pcap_max_read_packets = PCAP_FILE_MAX_PKTS;
> 
> Which corrects for the case where pcap_max_read_packets is 0. Im not
> sure if this is the best way to solve for the problem or is it better to
> initialize max_pending_packets to 1, which would still prevent
> pcap_dispatch from grabbing more than 256 packets as well.

I think it's a good extra line of defense but I think the real issue
lies with the question of max_pending_packets being 0. :)

Cheers,
Victor

> // Joel 
> 
> 
> 
> 
>> -----Original Message-----
>> From: Victor Julien [mailto:victor at inliniac.net]
>> Sent: Wednesday, September 08, 2010 6:47 AM
>> To: Joel Ebrahimi
>> Cc: oisf-devel at openinfosecfoundation.org
>> Subject: Re: [Oisf-devel] Packet Array in 1.0.x
>>
>> Joel Ebrahimi wrote:
>>> I have looked over the code and I don't see any checks that help. Let
> me
>>> explain the 2 problems I see.
>>>
>>> First when pcap_dispatch is called I have a large number of packets
> that
>>> are available. So this in turn calls PcapCallback. Now something
>>> PcapCallback is called 15,000 times in a row which will instantiate a
>>> lot of memory with the calls to PacketGetFromQueueOrAlloc(), so in
> this
>>> case I exhaust the entire memory of the system creating buffers for
>>> 15,000 packets. So this results in Linux killing the process.
>>>
>>> In the other case I do not exhaust all the memory, lets say
> PcapCallback
>>> is only called 12,000 times. Now in this case I return from the
> callback
>>> and move into the for loop. So at this point array_idx is 12,000. So
>>> when I enter the for loop cnt is then incremented up to 12,000. The
>>> problem here though is that the ptv->array only has 256 slots
> allocated
>>> for it, so since cnt exceeds 256 and there is an access to the
> variable
>>> ptv->array[cnt]; this will cause a segfault.
>>>
>>> Does that explain whats happening a little clearer? Im not sure why
> no
>>> one else has seen this yet, unless their buffers are small and the
>>> PcapCallback routine is called less than 256 times before processing.
>> The safety check that should catch this is on line source-pcap.c 197:
>>
>>        r = pcap_dispatch(ptv->pcap_handle, (pcap_max_read_packets <
>> packet_q_len) ? pcap_max_read_packets : packet_q_len,
>>            (pcap_handler)PcapCallback, (u_char *)ptv);
>>
>> pcap_max_read_packets should be equal to or lower than the array size.
>> It is initialized on line 252:
>>
>>    /* use max_pending_packets as pcap read size unless it's bigger
> than
>>     * our size limit */
>>    pcap_max_read_packets = (PCAP_FILE_MAX_PKTS < max_pending_packets)
> ?
>>        PCAP_FILE_MAX_PKTS : max_pending_packets;
>>
>> If all things operate like they should, pcap_max_read_packets should be
>> 256 (the default) in your case. If max_pending_packets is set to
>> something like 12000, pcap_max_read_packets should be 256 as it is
>> smaller than 12000.
>>
>> In the call to pcap_dispatch we check this variable against the
>> available packets in packet_q_len. If packet_q_len is bigger than
>> pcap_max_read_packets the value passed to pcap_dispatch should be that
>> of pcap_max_read_packets.
>>
>> In the backtrace of the segv you see, what are the values of
>> pcap_max_read_packets, packet_q_len, max_pending_packets, and what is
>> passed to pcap_dispatch?
>>
>> Cheers,
>> Victor
>>
>> --
>> ---------------------------------------------
>> Victor Julien
>> http://www.inliniac.net/
>> PGP: http://www.inliniac.net/victorjulien.asc
>> ---------------------------------------------
> 


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