[Oisf-devel] Packet Array in 1.0.x

Thu Sep 9 22:36:51 UTC 2010

I do not set max_pending_packets in the conf file, and I can see it is
commented out.

I see what the problem is though. You define max_pending_packets to be
of type intmax_t, where on my system architecture this is type long long
int. 

So it appears when the assignment of the regular int
DEFAULT_MAX_PENDING_PACKETS, the value of 50 is lost.

Also I noticed that when max_pending_packets is type mismatched to what
is defined externally in other files. In the external definitions it is
of type int.

When I change the type of max_pending_packets in suricata.c to an int,
everything works as it should even without those boundary checks I
added.

This also explains why this would work fine on some architectures and
others it would not.

I guess the fix would be just to change max_pending_packets to type int
in suricata.c. If you wanted to still include the range safety check
then I would actually suggest using:

    if(pcap_max_read_packets <= 0 || pcap_max_read_packets >
PCAP_FILE_MAX_PKTS )
        pcap_max_read_packets = PCAP_FILE_MAX_PKTS;

Instead of just checking for 0, that I had included in the previous
email. Ive attached a patch for the 1.0.2 branch with both of these
changes.

Cheers,

// Joel 

>-----Original Message-----
>From: Victor Julien [mailto:victor at inliniac.net]
>Sent: Wednesday, September 08, 2010 11:32 PM
>To: Joel Ebrahimi
>Cc: oisf-devel at openinfosecfoundation.org
>Subject: Re: [Oisf-devel] Packet Array in 1.0.x
>
>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 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
>---------------------------------------------

-------------- next part --------------
A non-text attachment was scrubbed...
Name: suricata-1.0.2.diff
Type: application/octet-stream
Size: 1472 bytes
Desc: suricata-1.0.2.diff
URL: <http://lists.openinfosecfoundation.org/pipermail/oisf-devel/attachments/20100909/91dffdd6/attachment.obj>