[dpdk-stable] [PATCH] librte_flow_classify: fix out-of-bounds access

[dpdk-stable] [PATCH] librte_flow_classify: fix out-of-bounds access

[Bernard Iremonger]

This patch fixes the out-of-bounds coverity issue by removing the
offending line of code at line 107 in rte_flow_classify_parse.c
which is never executed.

Coverity issue: 343454

Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
Cc: stable@dpdk.org
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_flow_classify/rte_flow_classify_parse.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_flow_classify/rte_flow_classify_parse.c b/lib/librte_flow_classify/rte_flow_classify_parse.c
index f65ceaf..4653302 100644
--- a/lib/librte_flow_classify/rte_flow_classify_parse.c
+++ b/lib/librte_flow_classify/rte_flow_classify_parse.c
@@ -103,8 +103,6 @@ classify_pattern_skip_void_item(struct rte_flow_item *items,
 			pb = pe;
 			break;
 		}
-
-		pb = pe + 1;
 	}
 	/* Copy the END item. */
 	rte_memcpy(items, pe, sizeof(struct rte_flow_item));
-- 
2.7.4

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Thomas Monjalon]

09/07/2019 13:09, Bernard Iremonger:
> This patch fixes the out-of-bounds coverity issue by removing the
> offending line of code at line 107 in rte_flow_classify_parse.c
> which is never executed.
> 
> Coverity issue: 343454
> 
> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
> Cc: stable@dpdk.org
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>

Applied, thanks

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[David Marchand]

On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 09/07/2019 13:09, Bernard Iremonger:
> > This patch fixes the out-of-bounds coverity issue by removing the
> > offending line of code at line 107 in rte_flow_classify_parse.c
> > which is never executed.
> >
> > Coverity issue: 343454
> >
> > Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
> > Cc: stable@dpdk.org
> > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>
> Applied, thanks

We have a segfault in the unit tests since this patch.


-- 
David Marchand

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Aaron Conole]

David Marchand <david.marchand@redhat.com> writes:

> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> 09/07/2019 13:09, Bernard Iremonger:
>> > This patch fixes the out-of-bounds coverity issue by removing the
>> > offending line of code at line 107 in rte_flow_classify_parse.c
>> > which is never executed.
>> >
>> > Coverity issue: 343454
>> >
>> > Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>> > Cc: stable@dpdk.org
>> > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>
>> Applied, thanks
>
> We have a segfault in the unit tests since this patch.

I think this patch is still correct.  The issue is in the semantic of
the flow classify pattern.  It *MUST* always have a valid end marker,
but the test passes an invalid end marker.  This causes the bounds to
exceed.

So, it would be best to fix it, either by having a "failure" on unknown
markers (f.e. -1), or by passing a length around.  However, the crash
should be expected.  The fact that the previous code was also incorrect
and resulted in no segfault is pure luck.

See rte_flow_classify_parse.c:80 and test_flow_classify.c:387

I would be in favor of passing the lengths of the two arrays to these
APIs.  That would let us still make use of the markers (for valid
construction), but also let us reason about lengths in a sane way.

WDYT?

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Ferruh Yigit]

On 7/29/2019 2:09 PM, David Marchand wrote:
> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> 09/07/2019 13:09, Bernard Iremonger:
>>> This patch fixes the out-of-bounds coverity issue by removing the
>>> offending line of code at line 107 in rte_flow_classify_parse.c
>>> which is never executed.
>>>
>>> Coverity issue: 343454
>>>
>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>>> Cc: stable@dpdk.org
>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>
>> Applied, thanks
> 
> We have a segfault in the unit tests since this patch.
> 

Yes, Flavia able to reproduce the crash.

That testcase is testing a pattern without invalid END item, the pattern is same
as the rte_flow pattern.

Expectation is 'rte_flow_classify_validate()' function detect this wrong pattern
and return error, but this can't happen.
Function gets pointer to the patter array without any size/length information,
function walks through the list until it detects the END item, if this item is
missing there is no way to limit the walk through within the boundaries of the
array. As far as I can see this is same in the rte_flow implementation.

An invalid patter with missing END item is not valid testcase with current
implementation, I guess it wasn't crashing before by luck, unless I am missing
something here.

I suggest removing the mentioned testcase, also remove similar testcase for
action, invalid action without END action. If the API supports this later we can
add back the testcases.

Thanks,
ferruh

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Ferruh Yigit]

On 7/30/2019 3:42 PM, Aaron Conole wrote:
> David Marchand <david.marchand@redhat.com> writes:
> 
>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>
>>> 09/07/2019 13:09, Bernard Iremonger:
>>>> This patch fixes the out-of-bounds coverity issue by removing the
>>>> offending line of code at line 107 in rte_flow_classify_parse.c
>>>> which is never executed.
>>>>
>>>> Coverity issue: 343454
>>>>
>>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>>>> Cc: stable@dpdk.org
>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>>
>>> Applied, thanks
>>
>> We have a segfault in the unit tests since this patch.
> 
> I think this patch is still correct.  The issue is in the semantic of
> the flow classify pattern.  It *MUST* always have a valid end marker,
> but the test passes an invalid end marker.  This causes the bounds to
> exceed.
> 
> So, it would be best to fix it, either by having a "failure" on unknown
> markers (f.e. -1), or by passing a length around.  However, the crash
> should be expected.  The fact that the previous code was also incorrect
> and resulted in no segfault is pure luck.
> 
> See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
> 
> I would be in favor of passing the lengths of the two arrays to these
> APIs.  That would let us still make use of the markers (for valid
> construction), but also let us reason about lengths in a sane way.
> 
> WDYT?
> 

+1, I also just replied with something very similar.

With current API the testcase is wrong, and it will crash, also the invalid
action one has exact same problem.

The API can be updated as you suggested, with a length field and testcases can
be added back.

What worries me more is the rte_flow, which uses same arguments, and open to
same errors, should we consider updating rte_flow APIs to have lengths values too?

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Aaron Conole]

Ferruh Yigit <ferruh.yigit@intel.com> writes:

> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>> David Marchand <david.marchand@redhat.com> writes:
>> 
>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>
>>>> 09/07/2019 13:09, Bernard Iremonger:
>>>>> This patch fixes the out-of-bounds coverity issue by removing the
>>>>> offending line of code at line 107 in rte_flow_classify_parse.c
>>>>> which is never executed.
>>>>>
>>>>> Coverity issue: 343454
>>>>>
>>>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>>>>> Cc: stable@dpdk.org
>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>>>
>>>> Applied, thanks
>>>
>>> We have a segfault in the unit tests since this patch.
>> 
>> I think this patch is still correct.  The issue is in the semantic of
>> the flow classify pattern.  It *MUST* always have a valid end marker,
>> but the test passes an invalid end marker.  This causes the bounds to
>> exceed.
>> 
>> So, it would be best to fix it, either by having a "failure" on unknown
>> markers (f.e. -1), or by passing a length around.  However, the crash
>> should be expected.  The fact that the previous code was also incorrect
>> and resulted in no segfault is pure luck.
>> 
>> See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
>> 
>> I would be in favor of passing the lengths of the two arrays to these
>> APIs.  That would let us still make use of the markers (for valid
>> construction), but also let us reason about lengths in a sane way.
>> 
>> WDYT?
>> 
>
> +1, I also just replied with something very similar.
>
> With current API the testcase is wrong, and it will crash, also the invalid
> action one has exact same problem.
>
> The API can be updated as you suggested, with a length field and testcases can
> be added back.
>
> What worries me more is the rte_flow, which uses same arguments, and open to
> same errors, should we consider updating rte_flow APIs to have lengths values too?

Probably.

Here's a first crack at the change I think is appropriate.  I have done
some limited testing.  Let me know if you want me to submit it formally.

---------------------------- 8< ---------------------------------
Subject: [PATCH] rte_flow_classify: fix up the API and preserve ABI

Introduces a new API for doing length validations, and preserves the old semantics
and API.  The previous API couldn't handle corrupted end markers.  A future
version of the API might be able to eschew the end marker and trust the length,
but that is a discussion for future.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 app/test/test_flow_classify.c                | 30 +-------
 lib/librte_flow_classify/rte_flow_classify.c | 72 +++++++++++++++++---
 lib/librte_flow_classify/rte_flow_classify.h | 28 ++++++++
 3 files changed, 91 insertions(+), 39 deletions(-)

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 6bbaad364..ff5265c6a 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -125,7 +125,6 @@ static struct rte_flow_item  udp_item_bad = { RTE_FLOW_ITEM_TYPE_UDP,
 
 static struct rte_flow_item  end_item = { RTE_FLOW_ITEM_TYPE_END,
 	0, 0, 0 };
-static struct rte_flow_item  end_item_bad = { -1, 0, 0, 0 };
 
 /* test TCP pattern:
  * "eth / ipv4 src spec 1.2.3.4 src mask 255.255.255.00 dst spec 5.6.7.8
@@ -181,7 +180,6 @@ static struct rte_flow_action count_action = { RTE_FLOW_ACTION_TYPE_COUNT,
 static struct rte_flow_action count_action_bad = { -1, 0};
 
 static struct rte_flow_action end_action = { RTE_FLOW_ACTION_TYPE_END, 0};
-static struct rte_flow_action end_action_bad =	{ -1, 0};
 
 static struct rte_flow_action actions[2];
 
@@ -384,7 +382,7 @@ test_invalid_patterns(void)
 
 	pattern[1] = ipv4_udp_item_1;
 	pattern[2] = udp_item_bad;
-	pattern[3] = end_item_bad;
+	pattern[3] = end_item;
 
 	ret = rte_flow_classify_validate(cls->cls, &attr, pattern,
 			actions, &error);
@@ -458,32 +456,6 @@ test_invalid_actions(void)
 		return -1;
 	}
 
-	actions[0] = count_action;
-	actions[1] = end_action_bad;
-
-	ret = rte_flow_classify_validate(cls->cls, &attr, pattern,
-			actions, &error);
-	if (!ret) {
-		printf("Line %i: rte_flow_classify_validate", __LINE__);
-		printf(" should have failed!\n");
-		return -1;
-	}
-
-	rule = rte_flow_classify_table_entry_add(cls->cls, &attr, pattern,
-			actions, &key_found, &error);
-	if (rule) {
-		printf("Line %i: flow_classify_table_entry_add", __LINE__);
-		printf(" should have failed!\n");
-		return -1;
-	}
-
-	ret = rte_flow_classify_table_entry_delete(cls->cls, rule);
-	if (!ret) {
-		printf("Line %i: rte_flow_classify_table_entry_delete",
-			__LINE__);
-		printf("should have failed!\n");
-		return -1;
-	}
 	return 0;
 }
 
diff --git a/lib/librte_flow_classify/rte_flow_classify.c b/lib/librte_flow_classify/rte_flow_classify.c
index 5ff585803..3ca1b1b44 100644
--- a/lib/librte_flow_classify/rte_flow_classify.c
+++ b/lib/librte_flow_classify/rte_flow_classify.c
@@ -89,18 +89,51 @@ struct rte_flow_classify_rule {
 	void *entry_ptr; /* handle to the table entry for rule meta data */
 };
 
+static size_t
+calc_flow_item_alen(const struct rte_flow_item pattern[])
+{
+	size_t i = 0;
+	while (pattern && (pattern + i)->type != RTE_FLOW_ITEM_TYPE_END)
+		i++;
+	return i + 1;
+}
+
+static size_t
+calc_flow_action_alen(const struct rte_flow_action actions[])
+{
+	size_t i = 0;
+	while (actions && (actions + i)->type != RTE_FLOW_ACTION_TYPE_END)
+		i++;
+	return i + 1;
+}
+
+int
+rte_flow_classify_validate(struct rte_flow_classifier *cls,
+			   const struct rte_flow_attr *attr,
+			   const struct rte_flow_item pattern[],
+			   const struct rte_flow_action actions[],
+			   struct rte_flow_error *error)
+{
+	return rte_flow_classify_validate_l(cls, attr, pattern,
+					    calc_flow_item_alen(pattern),
+					    actions,
+					    calc_flow_action_alen(actions),
+					    error);
+}
+
 int
-rte_flow_classify_validate(
-		   struct rte_flow_classifier *cls,
-		   const struct rte_flow_attr *attr,
-		   const struct rte_flow_item pattern[],
-		   const struct rte_flow_action actions[],
-		   struct rte_flow_error *error)
+rte_flow_classify_validate_l(struct rte_flow_classifier *cls,
+			     const struct rte_flow_attr *attr,
+			     const struct rte_flow_item pattern[],
+			     size_t pattern_l,
+			     const struct rte_flow_action actions[],
+			     size_t actions_l,
+			     struct rte_flow_error *error)
 {
 	struct rte_flow_item *items;
 	parse_filter_t parse_filter;
 	uint32_t item_num = 0;
-	uint32_t i = 0;
+	size_t i = 0;
 	int ret;
 
 	if (error == NULL)
@@ -134,17 +167,37 @@ rte_flow_classify_validate(
 		return -EINVAL;
 	}
 
+	while (i < actions_l && (actions + i)->type != RTE_FLOW_ACTION_TYPE_END)
+		i++;
+
+	if (i == actions_l) {
+		rte_flow_error_set(error, EINVAL,
+				   RTE_FLOW_ERROR_TYPE_ACTION_NUM,
+				   NULL, "Actions without end marker.");
+		return -EINVAL;
+	}
+
+	i = 0;
+
 	memset(&cls->ntuple_filter, 0, sizeof(cls->ntuple_filter));
 
 	/* Get the non-void item number of pattern */
-	while ((pattern + i)->type != RTE_FLOW_ITEM_TYPE_END) {
+	while (i < pattern_l && (pattern + i)->type != RTE_FLOW_ITEM_TYPE_END) {
 		if ((pattern + i)->type != RTE_FLOW_ITEM_TYPE_VOID)
 			item_num++;
 		i++;
 	}
+
 	item_num++;
 
-	items = malloc(item_num * sizeof(struct rte_flow_item));
+	if (i == pattern_l) {
+		rte_flow_error_set(error, EINVAL,
+				   RTE_FLOW_ERROR_TYPE_ITEM,
+				   NULL, "Pattern without end marker.");
+		return -EINVAL;
+	}
+
+	items = calloc(item_num, sizeof(struct rte_flow_item));
 	if (!items) {
 		rte_flow_error_set(error, ENOMEM,
 				RTE_FLOW_ERROR_TYPE_ITEM_NUM,
@@ -152,7 +205,6 @@ rte_flow_classify_validate(
 		return -ENOMEM;
 	}
 
-	memset(items, 0, item_num * sizeof(struct rte_flow_item));
 	classify_pattern_skip_void_item(items, pattern);
 
 	parse_filter = classify_find_parse_filter_func(items);
diff --git a/lib/librte_flow_classify/rte_flow_classify.h b/lib/librte_flow_classify/rte_flow_classify.h
index 74d1ecaf5..0308f6fd2 100644
--- a/lib/librte_flow_classify/rte_flow_classify.h
+++ b/lib/librte_flow_classify/rte_flow_classify.h
@@ -186,6 +186,34 @@ int
 rte_flow_classify_table_create(struct rte_flow_classifier *cls,
 		struct rte_flow_classify_table_params *params);
 
+/**
+ * Flow classify validate
+ *
+ * @param cls
+ *   Handle to flow classifier instance
+ * @param[in] attr
+ *   Flow rule attributes
+ * @param[in] pattern
+ *   Pattern specification (list terminated by the END pattern item).
+ * @param[in] actions
+ *   Associated actions (list terminated by the END pattern item).
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. Structure
+ *   initialised in case of error only.
+ * @return
+ *   0 on success, error code otherwise
+ */
+__rte_experimental
+int
+rte_flow_classify_validate_l(struct rte_flow_classifier *cls,
+			     const struct rte_flow_attr *attr,
+			     const struct rte_flow_item pattern[],
+			     const size_t pattern_l,
+			     const struct rte_flow_action actions[],
+			     const size_t actions_l,
+			     struct rte_flow_error *error);
+
+
 /**
  * Flow classify validate
  *
-- 
2.21.0

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Adrien Mazarguil]

On Tue, Jul 30, 2019 at 03:48:31PM +0100, Ferruh Yigit wrote:
> On 7/30/2019 3:42 PM, Aaron Conole wrote:
> > David Marchand <david.marchand@redhat.com> writes:
> > 
> >> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>>
> >>> 09/07/2019 13:09, Bernard Iremonger:
> >>>> This patch fixes the out-of-bounds coverity issue by removing the
> >>>> offending line of code at line 107 in rte_flow_classify_parse.c
> >>>> which is never executed.
> >>>>
> >>>> Coverity issue: 343454
> >>>>
> >>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
> >>>> Cc: stable@dpdk.org
> >>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> >>>
> >>> Applied, thanks
> >>
> >> We have a segfault in the unit tests since this patch.
> > 
> > I think this patch is still correct.  The issue is in the semantic of
> > the flow classify pattern.  It *MUST* always have a valid end marker,
> > but the test passes an invalid end marker.  This causes the bounds to
> > exceed.
> > 
> > So, it would be best to fix it, either by having a "failure" on unknown
> > markers (f.e. -1), or by passing a length around.  However, the crash
> > should be expected.  The fact that the previous code was also incorrect
> > and resulted in no segfault is pure luck.
> > 
> > See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
> > 
> > I would be in favor of passing the lengths of the two arrays to these
> > APIs.  That would let us still make use of the markers (for valid
> > construction), but also let us reason about lengths in a sane way.
> > 
> > WDYT?
> > 
> 
> +1, I also just replied with something very similar.
> 
> With current API the testcase is wrong, and it will crash, also the invalid
> action one has exact same problem.
> 
> The API can be updated as you suggested, with a length field and testcases can
> be added back.
> 
> What worries me more is the rte_flow, which uses same arguments, and open to
> same errors, should we consider updating rte_flow APIs to have lengths values too?

(Jumping in since all dashboard lights in my control room went red after
"rte_flow" was detected in this discussion)

Length values for patterns and action lists were considered during design
but END was preferred as the better solution for convenience and because
it's actually safer:

- C programmers are well aware of the dire consequences of omitting the
  ending NUL byte in strings so it's not a foreign concept. This is
  documented as such for rte_flow.

- Static initialization of flow rules (i.e. defining a large fixed array)
  is much easier if one doesn't have to encode its size as well, think about
  compilation directives (#ifdef) on some of its elements.

- Like omitting the END element, providing the wrong array size by mistake
  remains a possibility, with similar or possibly worse consequences as
  it's less likely to crash early and more prone to silent data corruption.

- [tons of other good reasons here]

See?

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Ferruh Yigit]

On 7/30/2019 5:18 PM, Adrien Mazarguil wrote:
> On Tue, Jul 30, 2019 at 03:48:31PM +0100, Ferruh Yigit wrote:
>> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>>> David Marchand <david.marchand@redhat.com> writes:
>>>
>>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>>
>>>>> 09/07/2019 13:09, Bernard Iremonger:
>>>>>> This patch fixes the out-of-bounds coverity issue by removing the
>>>>>> offending line of code at line 107 in rte_flow_classify_parse.c
>>>>>> which is never executed.
>>>>>>
>>>>>> Coverity issue: 343454
>>>>>>
>>>>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>>>>>> Cc: stable@dpdk.org
>>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>>>>
>>>>> Applied, thanks
>>>>
>>>> We have a segfault in the unit tests since this patch.
>>>
>>> I think this patch is still correct.  The issue is in the semantic of
>>> the flow classify pattern.  It *MUST* always have a valid end marker,
>>> but the test passes an invalid end marker.  This causes the bounds to
>>> exceed.
>>>
>>> So, it would be best to fix it, either by having a "failure" on unknown
>>> markers (f.e. -1), or by passing a length around.  However, the crash
>>> should be expected.  The fact that the previous code was also incorrect
>>> and resulted in no segfault is pure luck.
>>>
>>> See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
>>>
>>> I would be in favor of passing the lengths of the two arrays to these
>>> APIs.  That would let us still make use of the markers (for valid
>>> construction), but also let us reason about lengths in a sane way.
>>>
>>> WDYT?
>>>
>>
>> +1, I also just replied with something very similar.
>>
>> With current API the testcase is wrong, and it will crash, also the invalid
>> action one has exact same problem.
>>
>> The API can be updated as you suggested, with a length field and testcases can
>> be added back.
>>
>> What worries me more is the rte_flow, which uses same arguments, and open to
>> same errors, should we consider updating rte_flow APIs to have lengths values too?
> 
> (Jumping in since all dashboard lights in my control room went red after
> "rte_flow" was detected in this discussion)

:)

> 
> Length values for patterns and action lists were considered during design
> but END was preferred as the better solution for convenience and because
> it's actually safer:
> 
> - C programmers are well aware of the dire consequences of omitting the
>   ending NUL byte in strings so it's not a foreign concept. This is
>   documented as such for rte_flow.

I believe, C string functions are one of the most error prone part of the libc,
even after a dozen of years it is not rare to crash the applications because of
omitted terminating NULL, so I think this is not the best example :)

> 
> - Static initialization of flow rules (i.e. defining a large fixed array)
>   is much easier if one doesn't have to encode its size as well, think about
>   compilation directives (#ifdef) on some of its elements.
> 
> - Like omitting the END element, providing the wrong array size by mistake
>   remains a possibility, with similar or possibly worse consequences as
>   it's less likely to crash early and more prone to silent data corruption.

It is easy to pass the array length, sizeof(...), and this can prevent API to
walk through beyond the pattern array.
And having the END withing the array can be verified in API level before passing
the data to the drivers, so driver interface and code can stay intact.

> 
> - [tons of other good reasons here]
> 
> See?
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Ferruh Yigit]

On 7/30/2019 4:43 PM, Aaron Conole wrote:
> Ferruh Yigit <ferruh.yigit@intel.com> writes:
> 
>> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>>> David Marchand <david.marchand@redhat.com> writes:
>>>
>>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>>
>>>>> 09/07/2019 13:09, Bernard Iremonger:
>>>>>> This patch fixes the out-of-bounds coverity issue by removing the
>>>>>> offending line of code at line 107 in rte_flow_classify_parse.c
>>>>>> which is never executed.
>>>>>>
>>>>>> Coverity issue: 343454
>>>>>>
>>>>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>>>>>> Cc: stable@dpdk.org
>>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>>>>
>>>>> Applied, thanks
>>>>
>>>> We have a segfault in the unit tests since this patch.
>>>
>>> I think this patch is still correct.  The issue is in the semantic of
>>> the flow classify pattern.  It *MUST* always have a valid end marker,
>>> but the test passes an invalid end marker.  This causes the bounds to
>>> exceed.
>>>
>>> So, it would be best to fix it, either by having a "failure" on unknown
>>> markers (f.e. -1), or by passing a length around.  However, the crash
>>> should be expected.  The fact that the previous code was also incorrect
>>> and resulted in no segfault is pure luck.
>>>
>>> See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
>>>
>>> I would be in favor of passing the lengths of the two arrays to these
>>> APIs.  That would let us still make use of the markers (for valid
>>> construction), but also let us reason about lengths in a sane way.
>>>
>>> WDYT?
>>>
>>
>> +1, I also just replied with something very similar.
>>
>> With current API the testcase is wrong, and it will crash, also the invalid
>> action one has exact same problem.
>>
>> The API can be updated as you suggested, with a length field and testcases can
>> be added back.
>>
>> What worries me more is the rte_flow, which uses same arguments, and open to
>> same errors, should we consider updating rte_flow APIs to have lengths values too?
> 
> Probably.
> 
> Here's a first crack at the change I think is appropriate.  I have done
> some limited testing.  Let me know if you want me to submit it formally.
> 
> ---------------------------- 8< ---------------------------------
> Subject: [PATCH] rte_flow_classify: fix up the API and preserve ABI
> 
> Introduces a new API for doing length validations, and preserves the old semantics
> and API.  The previous API couldn't handle corrupted end markers.  A future
> version of the API might be able to eschew the end marker and trust the length,
> but that is a discussion for future.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  app/test/test_flow_classify.c                | 30 +-------
>  lib/librte_flow_classify/rte_flow_classify.c | 72 +++++++++++++++++---
>  lib/librte_flow_classify/rte_flow_classify.h | 28 ++++++++
>  3 files changed, 91 insertions(+), 39 deletions(-)
> 
> diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
> index 6bbaad364..ff5265c6a 100644
> --- a/app/test/test_flow_classify.c
> +++ b/app/test/test_flow_classify.c
> @@ -125,7 +125,6 @@ static struct rte_flow_item  udp_item_bad = { RTE_FLOW_ITEM_TYPE_UDP,
>  
>  static struct rte_flow_item  end_item = { RTE_FLOW_ITEM_TYPE_END,
>  	0, 0, 0 };
> -static struct rte_flow_item  end_item_bad = { -1, 0, 0, 0 };
>  
>  /* test TCP pattern:
>   * "eth / ipv4 src spec 1.2.3.4 src mask 255.255.255.00 dst spec 5.6.7.8
> @@ -181,7 +180,6 @@ static struct rte_flow_action count_action = { RTE_FLOW_ACTION_TYPE_COUNT,
>  static struct rte_flow_action count_action_bad = { -1, 0};
>  
>  static struct rte_flow_action end_action = { RTE_FLOW_ACTION_TYPE_END, 0};
> -static struct rte_flow_action end_action_bad =	{ -1, 0};
>  
>  static struct rte_flow_action actions[2];
>  
> @@ -384,7 +382,7 @@ test_invalid_patterns(void)
>  
>  	pattern[1] = ipv4_udp_item_1;
>  	pattern[2] = udp_item_bad;
> -	pattern[3] = end_item_bad;
> +	pattern[3] = end_item;
>  
>  	ret = rte_flow_classify_validate(cls->cls, &attr, pattern,
>  			actions, &error);
> @@ -458,32 +456,6 @@ test_invalid_actions(void)
>  		return -1;
>  	}
>  
> -	actions[0] = count_action;
> -	actions[1] = end_action_bad;
> -
> -	ret = rte_flow_classify_validate(cls->cls, &attr, pattern,
> -			actions, &error);
> -	if (!ret) {
> -		printf("Line %i: rte_flow_classify_validate", __LINE__);
> -		printf(" should have failed!\n");
> -		return -1;
> -	}
> -
> -	rule = rte_flow_classify_table_entry_add(cls->cls, &attr, pattern,
> -			actions, &key_found, &error);
> -	if (rule) {
> -		printf("Line %i: flow_classify_table_entry_add", __LINE__);
> -		printf(" should have failed!\n");
> -		return -1;
> -	}
> -
> -	ret = rte_flow_classify_table_entry_delete(cls->cls, rule);
> -	if (!ret) {
> -		printf("Line %i: rte_flow_classify_table_entry_delete",
> -			__LINE__);
> -		printf("should have failed!\n");
> -		return -1;
> -	}
>  	return 0;
>  }

+1 to unit test updates, lgtm.

And I am for pushing the library updates to the next release, but please find a
few comments for now.


>  
> diff --git a/lib/librte_flow_classify/rte_flow_classify.c b/lib/librte_flow_classify/rte_flow_classify.c
> index 5ff585803..3ca1b1b44 100644
> --- a/lib/librte_flow_classify/rte_flow_classify.c
> +++ b/lib/librte_flow_classify/rte_flow_classify.c
> @@ -89,18 +89,51 @@ struct rte_flow_classify_rule {
>  	void *entry_ptr; /* handle to the table entry for rule meta data */
>  };
>  
> +static size_t
> +calc_flow_item_alen(const struct rte_flow_item pattern[])
> +{
> +	size_t i = 0;
> +	while (pattern && (pattern + i)->type != RTE_FLOW_ITEM_TYPE_END)
> +		i++;
> +	return i + 1;

I think better to send '0' if the pointer is NULL, (instead of 1)

<...>

> @@ -186,6 +186,34 @@ int
>  rte_flow_classify_table_create(struct rte_flow_classifier *cls,
>  		struct rte_flow_classify_table_params *params);
>  
> +/**
> + * Flow classify validate
> + *
> + * @param cls
> + *   Handle to flow classifier instance
> + * @param[in] attr
> + *   Flow rule attributes
> + * @param[in] pattern
> + *   Pattern specification (list terminated by the END pattern item).
> + * @param[in] actions
> + *   Associated actions (list terminated by the END pattern item).
> + * @param[out] error
> + *   Perform verbose error reporting if not NULL. Structure
> + *   initialised in case of error only.
> + * @return
> + *   0 on success, error code otherwise
> + */
> +__rte_experimental
> +int
> +rte_flow_classify_validate_l(struct rte_flow_classifier *cls,
> +			     const struct rte_flow_attr *attr,
> +			     const struct rte_flow_item pattern[],
> +			     const size_t pattern_l,
> +			     const struct rte_flow_action actions[],
> +			     const size_t actions_l,
> +			     struct rte_flow_error *error);

The doxygen comment is missing for 'pattern_l' & 'actions_l' but from code it is
number of items in the lists, this is duplication of the END marker information.
Instead, if those lengths are the length of the arrays will it be easier for the
user? So user won't need to calculate the item count but can pass the size of
the array. This still prevents API access out of the array.

Anyway, as suggested above lets not make these decisions just a few days before
the release, but just get the unit test fix for the release, does it make sense?

And if so, can you send the unit test patch?

Thanks,
ferruh

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Aaron Conole]

Ferruh Yigit <ferruh.yigit@intel.com> writes:

> On 7/30/2019 5:18 PM, Adrien Mazarguil wrote:
>> On Tue, Jul 30, 2019 at 03:48:31PM +0100, Ferruh Yigit wrote:
>>> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>>>> David Marchand <david.marchand@redhat.com> writes:
>>>>
>>>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>>>
>>>>>> 09/07/2019 13:09, Bernard Iremonger:
>>>>>>> This patch fixes the out-of-bounds coverity issue by removing the
>>>>>>> offending line of code at line 107 in rte_flow_classify_parse.c
>>>>>>> which is never executed.
>>>>>>>
>>>>>>> Coverity issue: 343454
>>>>>>>
>>>>>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>>>>>>> Cc: stable@dpdk.org
>>>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>>>>>
>>>>>> Applied, thanks
>>>>>
>>>>> We have a segfault in the unit tests since this patch.
>>>>
>>>> I think this patch is still correct.  The issue is in the semantic of
>>>> the flow classify pattern.  It *MUST* always have a valid end marker,
>>>> but the test passes an invalid end marker.  This causes the bounds to
>>>> exceed.
>>>>
>>>> So, it would be best to fix it, either by having a "failure" on unknown
>>>> markers (f.e. -1), or by passing a length around.  However, the crash
>>>> should be expected.  The fact that the previous code was also incorrect
>>>> and resulted in no segfault is pure luck.
>>>>
>>>> See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
>>>>
>>>> I would be in favor of passing the lengths of the two arrays to these
>>>> APIs.  That would let us still make use of the markers (for valid
>>>> construction), but also let us reason about lengths in a sane way.
>>>>
>>>> WDYT?
>>>>
>>>
>>> +1, I also just replied with something very similar.
>>>
>>> With current API the testcase is wrong, and it will crash, also the invalid
>>> action one has exact same problem.
>>>
>>> The API can be updated as you suggested, with a length field and testcases can
>>> be added back.
>>>
>>> What worries me more is the rte_flow, which uses same arguments, and open to
>>> same errors, should we consider updating rte_flow APIs to have lengths values too?
>> 
>> (Jumping in since all dashboard lights in my control room went red after
>> "rte_flow" was detected in this discussion)
>
> :)
>
>> 
>> Length values for patterns and action lists were considered during design
>> but END was preferred as the better solution for convenience and because
>> it's actually safer:
>> 
>> - C programmers are well aware of the dire consequences of omitting the
>>   ending NUL byte in strings so it's not a foreign concept. This is
>>   documented as such for rte_flow.
>
> I believe, C string functions are one of the most error prone part of the libc,
> even after a dozen of years it is not rare to crash the applications because of
> omitted terminating NULL, so I think this is not the best example :)

+1

>> 
>> - Static initialization of flow rules (i.e. defining a large fixed array)
>>   is much easier if one doesn't have to encode its size as well, think about
>>   compilation directives (#ifdef) on some of its elements.
>> 
>> - Like omitting the END element, providing the wrong array size by mistake
>>   remains a possibility, with similar or possibly worse consequences as
>>   it's less likely to crash early and more prone to silent data corruption.
>
> It is easy to pass the array length, sizeof(...), and this can prevent API to
> walk through beyond the pattern array.
> And having the END withing the array can be verified in API level before passing
> the data to the drivers, so driver interface and code can stay intact.

Encoding 'END' within the array can only be enforced as an application
semantic.

The size of the array is a program / system semantic.

They cannot be used interchangeably, and we certainly shouldn't omit the
system semantic.  Notice how we're fixing a case that was directly
because of a programmer doing "the wrong thing" and an API that cannot
protect against it in any fashion.  That's in spite of some of your very
first comments:

      because it's actually safer

It isn't.  People and programmers make mistakes.  It's easier and more
efficient to calculate the size of an array (ARRAY_SIZE() is a fairly
well known macro) and pass it around.  It's worse to _recalculate_ the
size of an array each time (exponential execution) and have to
constantly walk elements from the head.

I didn't see the discussions on the flow API but I would have been
really critical of passing flat arrays without a corresponding length.

>> 
>> - [tons of other good reasons here]

As Ferruh notes, there are *billions* of examples of C strings being a
problem, and they are conceptually no different (a flat array with an
embedded end marker).  I think there might be 'reasons,' but I would
hesitate to know any of them as 'good'.

>> See?
>> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Aaron Conole]

Ferruh Yigit <ferruh.yigit@intel.com> writes:

> On 7/30/2019 4:43 PM, Aaron Conole wrote:
>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>> 
>>> On 7/30/2019 3:42 PM, Aaron Conole wrote:
>>>> David Marchand <david.marchand@redhat.com> writes:
>>>>
>>>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>>>
>>>>>> 09/07/2019 13:09, Bernard Iremonger:
>>>>>>> This patch fixes the out-of-bounds coverity issue by removing the
>>>>>>> offending line of code at line 107 in rte_flow_classify_parse.c
>>>>>>> which is never executed.
>>>>>>>
>>>>>>> Coverity issue: 343454
>>>>>>>
>>>>>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
>>>>>>> Cc: stable@dpdk.org
>>>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>>>>>
>>>>>> Applied, thanks
>>>>>
>>>>> We have a segfault in the unit tests since this patch.
>>>>
>>>> I think this patch is still correct.  The issue is in the semantic of
>>>> the flow classify pattern.  It *MUST* always have a valid end marker,
>>>> but the test passes an invalid end marker.  This causes the bounds to
>>>> exceed.
>>>>
>>>> So, it would be best to fix it, either by having a "failure" on unknown
>>>> markers (f.e. -1), or by passing a length around.  However, the crash
>>>> should be expected.  The fact that the previous code was also incorrect
>>>> and resulted in no segfault is pure luck.
>>>>
>>>> See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
>>>>
>>>> I would be in favor of passing the lengths of the two arrays to these
>>>> APIs.  That would let us still make use of the markers (for valid
>>>> construction), but also let us reason about lengths in a sane way.
>>>>
>>>> WDYT?
>>>>
>>>
>>> +1, I also just replied with something very similar.
>>>
>>> With current API the testcase is wrong, and it will crash, also the invalid
>>> action one has exact same problem.
>>>
>>> The API can be updated as you suggested, with a length field and testcases can
>>> be added back.
>>>
>>> What worries me more is the rte_flow, which uses same arguments, and open to
>>> same errors, should we consider updating rte_flow APIs to have lengths values too?
>> 
>> Probably.
>> 
>> Here's a first crack at the change I think is appropriate.  I have done
>> some limited testing.  Let me know if you want me to submit it formally.
>> 
>> ---------------------------- 8< ---------------------------------
>> Subject: [PATCH] rte_flow_classify: fix up the API and preserve ABI
>> 
>> Introduces a new API for doing length validations, and preserves the old semantics
>> and API.  The previous API couldn't handle corrupted end markers.  A future
>> version of the API might be able to eschew the end marker and trust the length,
>> but that is a discussion for future.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  app/test/test_flow_classify.c                | 30 +-------
>>  lib/librte_flow_classify/rte_flow_classify.c | 72 +++++++++++++++++---
>>  lib/librte_flow_classify/rte_flow_classify.h | 28 ++++++++
>>  3 files changed, 91 insertions(+), 39 deletions(-)
>> 
>> diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
>> index 6bbaad364..ff5265c6a 100644
>> --- a/app/test/test_flow_classify.c
>> +++ b/app/test/test_flow_classify.c
>> @@ -125,7 +125,6 @@ static struct rte_flow_item  udp_item_bad = { RTE_FLOW_ITEM_TYPE_UDP,
>>  
>>  static struct rte_flow_item  end_item = { RTE_FLOW_ITEM_TYPE_END,
>>  	0, 0, 0 };
>> -static struct rte_flow_item  end_item_bad = { -1, 0, 0, 0 };
>>  
>>  /* test TCP pattern:
>>   * "eth / ipv4 src spec 1.2.3.4 src mask 255.255.255.00 dst spec 5.6.7.8
>> @@ -181,7 +180,6 @@ static struct rte_flow_action count_action = { RTE_FLOW_ACTION_TYPE_COUNT,
>>  static struct rte_flow_action count_action_bad = { -1, 0};
>>  
>>  static struct rte_flow_action end_action = { RTE_FLOW_ACTION_TYPE_END, 0};
>> -static struct rte_flow_action end_action_bad =	{ -1, 0};
>>  
>>  static struct rte_flow_action actions[2];
>>  
>> @@ -384,7 +382,7 @@ test_invalid_patterns(void)
>>  
>>  	pattern[1] = ipv4_udp_item_1;
>>  	pattern[2] = udp_item_bad;
>> -	pattern[3] = end_item_bad;
>> +	pattern[3] = end_item;
>>  
>>  	ret = rte_flow_classify_validate(cls->cls, &attr, pattern,
>>  			actions, &error);
>> @@ -458,32 +456,6 @@ test_invalid_actions(void)
>>  		return -1;
>>  	}
>>  
>> -	actions[0] = count_action;
>> -	actions[1] = end_action_bad;
>> -
>> -	ret = rte_flow_classify_validate(cls->cls, &attr, pattern,
>> -			actions, &error);
>> -	if (!ret) {
>> -		printf("Line %i: rte_flow_classify_validate", __LINE__);
>> -		printf(" should have failed!\n");
>> -		return -1;
>> -	}
>> -
>> -	rule = rte_flow_classify_table_entry_add(cls->cls, &attr, pattern,
>> -			actions, &key_found, &error);
>> -	if (rule) {
>> -		printf("Line %i: flow_classify_table_entry_add", __LINE__);
>> -		printf(" should have failed!\n");
>> -		return -1;
>> -	}
>> -
>> -	ret = rte_flow_classify_table_entry_delete(cls->cls, rule);
>> -	if (!ret) {
>> -		printf("Line %i: rte_flow_classify_table_entry_delete",
>> -			__LINE__);
>> -		printf("should have failed!\n");
>> -		return -1;
>> -	}
>>  	return 0;
>>  }
>
> +1 to unit test updates, lgtm.
>
> And I am for pushing the library updates to the next release, but please find a
> few comments for now.

Okay - I'll do that.  But we probably will need to note that these APIs
should get deprecated.  Not sure if that should happen in this release -
as the author of most of the code, maybe you would take care of that
part? :)

>
>>  
>> diff --git a/lib/librte_flow_classify/rte_flow_classify.c b/lib/librte_flow_classify/rte_flow_classify.c
>> index 5ff585803..3ca1b1b44 100644
>> --- a/lib/librte_flow_classify/rte_flow_classify.c
>> +++ b/lib/librte_flow_classify/rte_flow_classify.c
>> @@ -89,18 +89,51 @@ struct rte_flow_classify_rule {
>>  	void *entry_ptr; /* handle to the table entry for rule meta data */
>>  };
>>  
>> +static size_t
>> +calc_flow_item_alen(const struct rte_flow_item pattern[])
>> +{
>> +	size_t i = 0;
>> +	while (pattern && (pattern + i)->type != RTE_FLOW_ITEM_TYPE_END)
>> +		i++;
>> +	return i + 1;
>
> I think better to send '0' if the pointer is NULL, (instead of 1)

Okay.  Makes sense.

> <...>
>
>> @@ -186,6 +186,34 @@ int
>>  rte_flow_classify_table_create(struct rte_flow_classifier *cls,
>>  		struct rte_flow_classify_table_params *params);
>>  
>> +/**
>> + * Flow classify validate
>> + *
>> + * @param cls
>> + *   Handle to flow classifier instance
>> + * @param[in] attr
>> + *   Flow rule attributes
>> + * @param[in] pattern
>> + *   Pattern specification (list terminated by the END pattern item).
>> + * @param[in] actions
>> + *   Associated actions (list terminated by the END pattern item).
>> + * @param[out] error
>> + *   Perform verbose error reporting if not NULL. Structure
>> + *   initialised in case of error only.
>> + * @return
>> + *   0 on success, error code otherwise
>> + */
>> +__rte_experimental
>> +int
>> +rte_flow_classify_validate_l(struct rte_flow_classifier *cls,
>> +			     const struct rte_flow_attr *attr,
>> +			     const struct rte_flow_item pattern[],
>> +			     const size_t pattern_l,
>> +			     const struct rte_flow_action actions[],
>> +			     const size_t actions_l,
>> +			     struct rte_flow_error *error);
>
> The doxygen comment is missing for 'pattern_l' & 'actions_l' but from code it is
> number of items in the lists, this is duplication of the END marker information.
> Instead, if those lengths are the length of the arrays will it be easier for the
> user? So user won't need to calculate the item count but can pass the size of
> the array. This still prevents API access out of the array.
>
> Anyway, as suggested above lets not make these decisions just a few days before
> the release, but just get the unit test fix for the release, does it make sense?

Sure.

> And if so, can you send the unit test patch?

Will do.

> Thanks,
> ferruh

Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access

[Adrien Mazarguil]

On Tue, Jul 30, 2019 at 01:27:41PM -0400, Aaron Conole wrote:
> Ferruh Yigit <ferruh.yigit@intel.com> writes:
> 
> > On 7/30/2019 5:18 PM, Adrien Mazarguil wrote:
> >> On Tue, Jul 30, 2019 at 03:48:31PM +0100, Ferruh Yigit wrote:
> >>> On 7/30/2019 3:42 PM, Aaron Conole wrote:
> >>>> David Marchand <david.marchand@redhat.com> writes:
> >>>>
> >>>>> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>>>>>
> >>>>>> 09/07/2019 13:09, Bernard Iremonger:
> >>>>>>> This patch fixes the out-of-bounds coverity issue by removing the
> >>>>>>> offending line of code at line 107 in rte_flow_classify_parse.c
> >>>>>>> which is never executed.
> >>>>>>>
> >>>>>>> Coverity issue: 343454
> >>>>>>>
> >>>>>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
> >>>>>>> Cc: stable@dpdk.org
> >>>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> >>>>>>
> >>>>>> Applied, thanks
> >>>>>
> >>>>> We have a segfault in the unit tests since this patch.
> >>>>
> >>>> I think this patch is still correct.  The issue is in the semantic of
> >>>> the flow classify pattern.  It *MUST* always have a valid end marker,
> >>>> but the test passes an invalid end marker.  This causes the bounds to
> >>>> exceed.
> >>>>
> >>>> So, it would be best to fix it, either by having a "failure" on unknown
> >>>> markers (f.e. -1), or by passing a length around.  However, the crash
> >>>> should be expected.  The fact that the previous code was also incorrect
> >>>> and resulted in no segfault is pure luck.
> >>>>
> >>>> See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
> >>>>
> >>>> I would be in favor of passing the lengths of the two arrays to these
> >>>> APIs.  That would let us still make use of the markers (for valid
> >>>> construction), but also let us reason about lengths in a sane way.
> >>>>
> >>>> WDYT?
> >>>>
> >>>
> >>> +1, I also just replied with something very similar.
> >>>
> >>> With current API the testcase is wrong, and it will crash, also the invalid
> >>> action one has exact same problem.
> >>>
> >>> The API can be updated as you suggested, with a length field and testcases can
> >>> be added back.
> >>>
> >>> What worries me more is the rte_flow, which uses same arguments, and open to
> >>> same errors, should we consider updating rte_flow APIs to have lengths values too?
> >> 
> >> (Jumping in since all dashboard lights in my control room went red after
> >> "rte_flow" was detected in this discussion)
> >
> > :)
> >
> >> 
> >> Length values for patterns and action lists were considered during design
> >> but END was preferred as the better solution for convenience and because
> >> it's actually safer:
> >> 
> >> - C programmers are well aware of the dire consequences of omitting the
> >>   ending NUL byte in strings so it's not a foreign concept. This is
> >>   documented as such for rte_flow.
> >
> > I believe, C string functions are one of the most error prone part of the libc,
> > even after a dozen of years it is not rare to crash the applications because of
> > omitted terminating NULL, so I think this is not the best example :)
> 
> +1

Of course, but I see such crashes as a *feature* when something's wrong in
the code. Silent data corruption is much, much worse. Those are not
recoverable errors, so it's no different from ignoring SIGSEGV and hoping
for the best (whee, no more crashes!)

> >> 
> >> - Static initialization of flow rules (i.e. defining a large fixed array)
> >>   is much easier if one doesn't have to encode its size as well, think about
> >>   compilation directives (#ifdef) on some of its elements.
> >> 
> >> - Like omitting the END element, providing the wrong array size by mistake
> >>   remains a possibility, with similar or possibly worse consequences as
> >>   it's less likely to crash early and more prone to silent data corruption.
> >
> > It is easy to pass the array length, sizeof(...), and this can prevent API to
> > walk through beyond the pattern array.
> > And having the END withing the array can be verified in API level before passing
> > the data to the drivers, so driver interface and code can stay intact.
> 
> Encoding 'END' within the array can only be enforced as an application
> semantic.
> 
> The size of the array is a program / system semantic.
> 
> They cannot be used interchangeably, and we certainly shouldn't omit the
> system semantic.  Notice how we're fixing a case that was directly
> because of a programmer doing "the wrong thing" and an API that cannot
> protect against it in any fashion.  That's in spite of some of your very
> first comments:
> 
>       because it's actually safer
> 
> It isn't.  People and programmers make mistakes.  It's easier and more
> efficient to calculate the size of an array (ARRAY_SIZE() is a fairly
> well known macro) and pass it around.

ARRAY_SIZE() doesn't work with pointers, in which case a miscalculated size
when dynamically building/modifying flow rules is as much a possibility as a
missing END marker, in which case:

- A shorter size will usually translate to a valid flow rule that silently
  doesn't behave as expected.

- A moderately larger size will typically not crash, but whatever comes
  afterward in memory will be interpreted as part of that rule.

In both situations a crash would have been preferable (well, IMO).

> It's worse to _recalculate_ the
> size of an array each time (exponential execution) and have to
> constantly walk elements from the head.

I think there's also a misconception here, rte_flow patterns and action
lists are crafted in a way that makes their size irrelevant. PMDs are
expected to parse them as fast as possible in a *single pass* till they hit
END. Except for wasting CPU registers on additional arguments, knowing the
size in advance is useless to them.

> I didn't see the discussions on the flow API but I would have been
> really critical of passing flat arrays without a corresponding length.

Phew, I think this API would never have landed with us arguing forever about
that :)

I know the size can be useful to applications in some situations, be it for
allocations, housekeeping and whatnot, but PMDs really do not need it.

Applications are free to pass the size around for their own needs, but at
the DPDK API level, it's useless. I refuse to accept "hiding programming
mistakes" as a valid reason.

> 
> >> 
> >> - [tons of other good reasons here]
> 
> As Ferruh notes, there are *billions* of examples of C strings being a
> problem, and they are conceptually no different (a flat array with an
> embedded end marker).  I think there might be 'reasons,' but I would
> hesitate to know any of them as 'good'.

I only used C strings to illustrate the well-known terminating NUL approach,
but rte_flow thankfully won't provide as many manipulation functions as the
C string library, so the scope is quite limited here.

In practice flow rules are typically built on the stack by applications to
be immediately passed to PMDs, they are not expected to be processed much if
at all on their way. And if an application happens to need it, well it's
free to maintain as much metadata as required.

Looks like you dislike C strings for the wrong reasons. They're simple,
elegant, useful for their tendency to crash early when mishandled while not
preventing users from adding as much complexity as they like on top.

-- 
Adrien Mazarguil
6WIND