[Oisf-devel] Packet Array in 1.0.x

Thu Sep 16 15:22:10 UTC 2010

Ahh good catch Joel! I think the intmax_t was done because the Conf api
uses it to support any size of arguments.

Converting max_pending_packets to int might work, but internally
ConfGetInt still operates on a intmax_t, so that might have side effects
as well. Maybe the Conf api just needs a call for a normal int.

I think 4 billion (or 2 actually for a normal (signed) int) packets
should be enough for everyone :)

Cheers,
Victor

Joel Ebrahimi wrote:
> 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
>> ---------------------------------------------
> 

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