GCC7 said:
CC util-radix-tree.o
In file included from util-debug-filters.h:29:0,
from util-debug.h:34,
from suricata-common.h:421,
from util-radix-tree.c:26:
util-radix-tree.c: In function ‘SCRadixAddKey’:
util-mem.h:177:12: error: argument 1 range [18446744071562067968, 18446744073709551615] exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
ptrmem = malloc((a)); \
~~~~~~~^~~~~~~~~~~~~
util-radix-tree.c:749:42: note: in expansion of macro ‘SCMalloc’
if ( (inter_node->netmasks = SCMalloc((node->netmask_cnt - i) *
^~~~~~~~
In file included from suricata-common.h:69:0,
from util-radix-tree.c:26:
/usr/include/stdlib.h:443:14: note: in a call to allocation function ‘malloc’ declared here
extern void *malloc (size_t __size) __THROW __attribute_malloc__ __wur;
^~~~~~
scan-build said:
util-radix-tree.c:749:42: warning: Call to 'malloc' has an allocation size of 0 bytes
if ( (inter_node->netmasks = SCMalloc((node->netmask_cnt - i) *
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./util-mem.h:177:14: note: expanded from macro 'SCMalloc'
ptrmem = malloc((a)); \
^~~~~~~~~~~
1 warning generated.
Set flags by default:
-Wmissing-prototypes
-Wmissing-declarations
-Wstrict-prototypes
-Wwrite-strings
-Wcast-align
-Wbad-function-cast
-Wformat-security
-Wno-format-nonliteral
-Wmissing-format-attribute
-funsigned-char
Fix minor compiler warnings for these new flags on gcc and clang.
[src/util-ip.c:104]: (error) Shifting a negative value is undefined behaviour
[src/util-radix-tree.c:1160]: (error) Shifting a negative value is undefined behaviour
[src/util-radix-tree.c:1357]: (error) Shifting a negative value is undefined behaviour
[src/util-radix-tree.c:1380]: (error) Shifting a negative value is undefined behaviour
[src/util-radix-tree.c:1438]: (error) Shifting a negative value is undefined behaviour
Logic used when adding a new prefix to a node was not correct
as we were allocating a prefix that could be at the end unused.
This patch is updating the code to have a complete creation to
be done if and only if we are needing the complete object. In
the other cases, it was enough to use the function input values.
This fixes:
104 (48 direct, 56 indirect) bytes in 2 blocks are definitely lost in loss record 184 of 327
at 0x4C29C0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x9C2DAD: SCRadixCreatePrefix (util-radix-tree.c:144)
by 0x9AFA5B: SCRadixAddKey (util-radix-tree.c:522)
by 0x9B1A4D: SCRadixAddKeyIPV4Netblock (util-radix-tree.c:897)
by 0x67C824: IPOnlyPrepare (detect-engine-iponly.c:1197)
by 0x55172B: SigAddressPrepareStage2 (detect.c:3534)
by 0x5486F4: SigGroupBuild (detect.c:4671)
by 0x547C87: SigLoadSignatures (detect.c:538)
by 0x8FB5D0: LoadSignatures (suricata.c:1976)
by 0x8F3B32: main (suricata.c:2342)
An IPv6 entry specified before an IPv4 entry on the host-os-policy
table can cause the stream byte array to be access one byte after
the end of the allocated memory at util-radix-tree.c:578.
I found three somewhat serious IPv6 address bugs within the Suricata 2.0.x source code. Two are in the source module "detect-engine-address.c", and the third is in "util-radix-tree.c".
The first bug occurs within the function DetectAddressParse2(). When parsing an address string and a negated block is encountered (such as when parsing !$HOME_NET, for example), any corresponding IPv6 addresses were not getting added to the Group Heads in the DetectAddressList. Only IPv4 addresses were being added.
I discovered another bug related to IPv6 address ranges in the Signature Match Address Array comparison code for IPv6 addresses. The function DetectAddressMatchIPv6() walks a signature's source or destination match address list comparing each to the current packet's corresponding address value. The match address list consists of value pairs representing a lower and upper IP address range. If the packet's address is within that range (including equal to either the lower or upper bound), then a signature match flag is returned.
The original test of each signature match address to the packet was performed using a set of four compounded AND comparisons looking at each of the four 32-bit blocks that comprise an IPv6 address. The problem with the old comparison is that if ANY of the four 32-bit blocks failed the test, then a "no-match" was returned. This is incorrect. If one or more of the more significant 32-bit blocks met the condition, then it is a match no matter if some of the less significant 32-bit blocks did not meet the condition. Consider this example where Packet represents the packet address being checked, and Target represents the upper bound of a match address pair. We are testing if Packet is less than Target.
Packet -- 2001:0470 : 1f07:00e2 : 1988:01f1 : d468:27ab
Target -- 2001:0470 : 1f07:00e2 : a48c:2e52 : d121:101e
In this example the Packet's address is less than the target and it should give a match. However, the old code would compare each 32-bit block (shown spaced out above for clarity) and logically AND the result with the next least significant block comparison. If any of the four blocks failed the comparison, that kicked out the whole address. The flaw is illustrated above. The first two blocks are 2001:0470 and 1f07:00e2 and yield TRUE; the next less significant block is 1988:01f1 and a48c:2e52, and also yields TRUE (that is, Packet is less than Target); but the last block compare is FALSE (d468:27ab is not less than d121:101e). That last block is the least significant block, though, so its FALSE determination should not invalidate a TRUE from any of the more significant blocks. However, in the previous code using the compound logical AND block, that last least significant block would invalidate the tests done with the more significant blocks.
The other bug I found for IPv6 occurs when trying to parse and insert an IPv6 address into a Radix Tree using the function SCRadixAddKeyIPV6String(). The test for min and max values for an IPv6 CIDR mask incorrectly tests the upper limit as 32 when it should be 128 for an IPv6 address. I think this perhaps is an old copy-paste error if the IPv6 version of this function was initially copied from the corresponding IPv4 version directly above it in the code. Without this patch, the function will return null when you attempt to add an IPv6 network whose CIDR mask is larger than 32 (for example, the popular /64 mask will cause the function to return the NULL error condition).
(amended by Victor Julien)
This patch updates all the radix tests to the new API. In most cases
it just passes a NULL user data return pointer.
It also removes the tests related to SC_RADIX_NODE_USERDATA, as this
macro is removed.
Bug #1073
The radix tree stores user data. However, it had no function to return
this data to the consumers of the API. Instead, on lookup, it would
set a field "user_data_result" in the nodes prefix structure which
could then be read by the caller.
Apart for this not being a very nice design as it exposes API internals
to the caller, it is not thread safe. By updating the global data
structure without any form (or suggestion) of locking, threads could
overwrite the same field unexpectedly.
This patch modifies the lookup logic to get rid of this stored
user_data_result. Instead, all the lookup functions how take an
addition argument: void **user_data_result.
Through this pointer the user data is returned. It's allowed to be
NULL, in this case the user data is ignored.
This is a significant API change, that affects a lot of tests and
callers. These will be updated in follow up patches.
Bug #1073.
This patch fixes a compilation failure on Solaris. Compiler does
not support when a function returning void is used in return of
an other function returning void.
When handling error case on SCMallog, SCCalloc or SCStrdup
we are in an unlikely case. This patch adds the unlikely()
expression to indicate this to gcc.
This patch has been obtained via coccinelle. The transformation
is the following:
@istested@
identifier x;
statement S1;
identifier func =~ "(SCMalloc|SCStrdup|SCCalloc)";
@@
x = func(...)
... when != x
- if (x == NULL) S1
+ if (unlikely(x == NULL)) S1
This patch fixes compilation on OpenBSD platform. It is running
fine on a pcap file. The patch should also fix compilation on
WIN32 platform but this is not tested.
Victor Julien
Jason Ish
Eric Leblond
bmeeks8
Ken Steele
Duarte Silva
Anoop Saldanha
Pablo Rincon
Gurvinder Singh
William Metcalf
Gerardo Iglesias Galvan
Brian Rectanus