[v1] lib/eal: enforce alarm APIs parameters check
Checks
Commit Message
From: Jie Zhou <jizh@microsoft.com>
lib/eal alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
on Windows do not check parameters to fail fast for invalid
parameters, which captured by DPDK UT alarm_autotest.
Enforce Windows lib/eal alarm APIs parameters check and log
invalid parameter info.
Signed-off-by: Jie Zhou <jizh@microsoft.com>
Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
---
lib/eal/windows/eal_alarm.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
Comments
Hi Jie,
2021-06-23 17:36 (UTC-0700), Jie Zhou:
> From: Jie Zhou <jizh@microsoft.com>
>
> lib/eal alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
> on Windows do not check parameters to fail fast for invalid
> parameters, which captured by DPDK UT alarm_autotest.
Please use past tense to describe situation before the patch.
A nit, but browsing the log, I see that errors are usually "caught"
rather then "captured"; consistency would be nice.
>
> Enforce Windows lib/eal alarm APIs parameters check and log
> invalid parameter info.
Fixes tag needed.
> Signed-off-by: Jie Zhou <jizh@microsoft.com>
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
>
> ---
> lib/eal/windows/eal_alarm.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
> index f5bf88715a..7bb79ae869 100644
> --- a/lib/eal/windows/eal_alarm.c
> +++ b/lib/eal/windows/eal_alarm.c
> @@ -4,6 +4,7 @@
>
> #include <stdatomic.h>
> #include <stdbool.h>
> +#include <inttypes.h>
>
> #include <rte_alarm.h>
> #include <rte_spinlock.h>
> @@ -91,6 +92,22 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
> LARGE_INTEGER deadline;
> int ret;
>
> + /* Check if us is valid */
> + if (us < 1 || us >(UINT64_MAX - US_PER_S)) {
This condition is specific to Linux EAL. In fact, it's not very useful even
there, because actual upper bound for `us` depends on current time.
No bounds are specified in API description at all.
Windows check would be different, but these considerations remain valid.
Maybe it's alarm_autotest or API description that needs adjustments,
but not the implementation. I understand that you're enabling UT for Windows
and not correcting tests themselves, but I'm against inserting checks known
to be incorrect.
> + RTE_LOG(ERR, EAL, "Invalid us: %" PRIu64 "\n"
> + "Valid us range is 1 to (UINT64_MAX - US_PER_S)\n",
> + us);
Why does Windows need these messages, while Linux and FreeBSD don't?
How will printing API contract here help the user who gets the message?
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + /* Check if callback is not NULL */
> + if (!cb_fn) {
Pointers (`cb_fn`) must be checked for `NULL` explicitly.
You won't need an obvious comment after that.
> + RTE_LOG(ERR, EAL, "NULL callback\n");
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> /* Calculate deadline ASAP, unit of measure = 100ns. */
> GetSystemTimePreciseAsFileTime(&ft);
> deadline.LowPart = ft.dwLowDateTime;
> @@ -180,6 +197,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
> bool executing;
>
> removed = 0;
> +
> + if (!cb_fn) {
> + RTE_LOG(ERR, EAL, "NULL callback\n");
> + return -EINVAL;
> + }
> +
> do {
> executing = false;
>
Please also fix other style issues:
http://mails.dpdk.org/archives/test-report/2021-June/200580.html
On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote:
> Hi Jie,
>
> 2021-06-23 17:36 (UTC-0700), Jie Zhou:
> > From: Jie Zhou <jizh@microsoft.com>
> >
> > lib/eal alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
> > on Windows do not check parameters to fail fast for invalid
> > parameters, which captured by DPDK UT alarm_autotest.
>
> Please use past tense to describe situation before the patch.
> A nit, but browsing the log, I see that errors are usually "caught"
> rather then "captured"; consistency would be nice.
>
> >
> > Enforce Windows lib/eal alarm APIs parameters check and log
> > invalid parameter info.
>
> Fixes tag needed.
>
> > Signed-off-by: Jie Zhou <jizh@microsoft.com>
> > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> >
> > ---
> > lib/eal/windows/eal_alarm.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
> > index f5bf88715a..7bb79ae869 100644
> > --- a/lib/eal/windows/eal_alarm.c
> > +++ b/lib/eal/windows/eal_alarm.c
> > @@ -4,6 +4,7 @@
> >
> > #include <stdatomic.h>
> > #include <stdbool.h>
> > +#include <inttypes.h>
> >
> > #include <rte_alarm.h>
> > #include <rte_spinlock.h>
> > @@ -91,6 +92,22 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
> > LARGE_INTEGER deadline;
> > int ret;
> >
> > + /* Check if us is valid */
> > + if (us < 1 || us >(UINT64_MAX - US_PER_S)) {
>
> This condition is specific to Linux EAL. In fact, it's not very useful even
> there, because actual upper bound for `us` depends on current time.
> No bounds are specified in API description at all.
> Windows check would be different, but these considerations remain valid.
>
> Maybe it's alarm_autotest or API description that needs adjustments,
> but not the implementation.
i agree with your assessment. it's a bit silly to test a range
constraint where the range is just some arbitrary range and not the real
range. changing the implementation to calculate the "real" valid range
isn't practical. i'm sure linux implementors would argue that catching
some extremely out of range values is better than none?
i guess a correct test would would calculate the valid range and test
against that but as per above the implementation won't pass the test.
so we are left with
* matching the range imposed in the linux implementation so we pass the
test as is.
* don't run the test with the input data that exercises this bogus range
constraint.
i guess based on your comments you prefer the latter? so is our action
here to submit a patch for the test that doesn't test this range
conditionally on execenv windows?
> I understand that you're enabling UT for Windows
> and not correcting tests themselves, but I'm against inserting checks known
> to be incorrect.
>
> > + RTE_LOG(ERR, EAL, "Invalid us: %" PRIu64 "\n"
> > + "Valid us range is 1 to (UINT64_MAX - US_PER_S)\n",
> > + us);
>
> Why does Windows need these messages, while Linux and FreeBSD don't?
> How will printing API contract here help the user who gets the message?
i'll discuss this subject to whether or not we remove the above range
check. for now i'll defer discussion.
>
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + /* Check if callback is not NULL */
> > + if (!cb_fn) {
>
> Pointers (`cb_fn`) must be checked for `NULL` explicitly.
> You won't need an obvious comment after that.
>
> > + RTE_LOG(ERR, EAL, "NULL callback\n");
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > /* Calculate deadline ASAP, unit of measure = 100ns. */
> > GetSystemTimePreciseAsFileTime(&ft);
> > deadline.LowPart = ft.dwLowDateTime;
> > @@ -180,6 +197,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
> > bool executing;
> >
> > removed = 0;
> > +
> > + if (!cb_fn) {
> > + RTE_LOG(ERR, EAL, "NULL callback\n");
> > + return -EINVAL;
> > + }
> > +
> > do {
> > executing = false;
> >
>
> Please also fix other style issues:
> http://mails.dpdk.org/archives/test-report/2021-June/200580.html
2021-07-01 09:21 (UTC-0700), Tyler Retzlaff:
> On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote:
> > Hi Jie,
> >
> > 2021-06-23 17:36 (UTC-0700), Jie Zhou:
> > > From: Jie Zhou <jizh@microsoft.com>
> [...]
> > > + /* Check if us is valid */
> > > + if (us < 1 || us >(UINT64_MAX - US_PER_S)) {
> >
> > This condition is specific to Linux EAL. In fact, it's not very useful even
> > there, because actual upper bound for `us` depends on current time.
> > No bounds are specified in API description at all.
> > Windows check would be different, but these considerations remain valid.
> >
> > Maybe it's alarm_autotest or API description that needs adjustments,
> > but not the implementation.
>
> i agree with your assessment. it's a bit silly to test a range
> constraint where the range is just some arbitrary range and not the real
> range. changing the implementation to calculate the "real" valid range
> isn't practical. i'm sure linux implementors would argue that catching
> some extremely out of range values is better than none?
Why we care about overflow?
1. It likely indicates a bug in the app.
2. Alarm is can to be scheduled to some time in the past and fire
immediately, which means API did not do what it promises.
I see the only way to tell the interval is incorrect if it gives
(would give) time in the past when added to the current time.
But this is an implementation detail and does not need testing.
A separate patch can be submitted to change behavior for all OS.
>
> i guess a correct test would would calculate the valid range and test
> against that but as per above the implementation won't pass the test.
>
> so we are left with
>
> * matching the range imposed in the linux implementation so we pass the
> test as is.
It would be the worst thing one could do, defying the purpose of unit tests.
> * don't run the test with the input data that exercises this bogus range
> constraint.
>
> i guess based on your comments you prefer the latter? so is our action
> here to submit a patch for the test that doesn't test this range
> conditionally on execenv windows?
Yes, please remove this check from the test as it verifies no contract.
On Fri, Jul 02, 2021 at 12:36:45AM +0300, Dmitry Kozlyuk wrote:
> 2021-07-01 09:21 (UTC-0700), Tyler Retzlaff:
> > On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote:
> > > Hi Jie,
> > >
> > > 2021-06-23 17:36 (UTC-0700), Jie Zhou:
> > > > From: Jie Zhou <jizh@microsoft.com>
> > [...]
> > > > + /* Check if us is valid */
> > > > + if (us < 1 || us >(UINT64_MAX - US_PER_S)) {
> > >
> > > This condition is specific to Linux EAL. In fact, it's not very useful even
> > > there, because actual upper bound for `us` depends on current time.
> > > No bounds are specified in API description at all.
> > > Windows check would be different, but these considerations remain valid.
> > >
> > > Maybe it's alarm_autotest or API description that needs adjustments,
> > > but not the implementation.
> >
> > i agree with your assessment. it's a bit silly to test a range
> > constraint where the range is just some arbitrary range and not the real
> > range. changing the implementation to calculate the "real" valid range
> > isn't practical. i'm sure linux implementors would argue that catching
> > some extremely out of range values is better than none?
>
> Why we care about overflow?
> 1. It likely indicates a bug in the app.
> 2. Alarm is can to be scheduled to some time in the past and fire
> immediately, which means API did not do what it promises.
> I see the only way to tell the interval is incorrect if it gives
> (would give) time in the past when added to the current time.
> But this is an implementation detail and does not need testing.
> A separate patch can be submitted to change behavior for all OS.
>
> >
> > i guess a correct test would would calculate the valid range and test
> > against that but as per above the implementation won't pass the test.
> >
> > so we are left with
> >
> > * matching the range imposed in the linux implementation so we pass the
> > test as is.
>
> It would be the worst thing one could do, defying the purpose of unit tests.
agreed.
>
> > * don't run the test with the input data that exercises this bogus range
> > constraint.
> >
> > i guess based on your comments you prefer the latter? so is our action
> > here to submit a patch for the test that doesn't test this range
> > conditionally on execenv windows?
>
> Yes, please remove this check from the test as it verifies no contract.
we'll go with this.
thanks
On Thu, Jul 01, 2021 at 02:45:24PM -0700, Tyler Retzlaff wrote:
> On Fri, Jul 02, 2021 at 12:36:45AM +0300, Dmitry Kozlyuk wrote:
> > 2021-07-01 09:21 (UTC-0700), Tyler Retzlaff:
> > > On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote:
> > > > Hi Jie,
> > > >
> > > > 2021-06-23 17:36 (UTC-0700), Jie Zhou:
> > > > > From: Jie Zhou <jizh@microsoft.com>
> > > [...]
> > > > > + /* Check if us is valid */
> > > > > + if (us < 1 || us >(UINT64_MAX - US_PER_S)) {
> > > >
> > > > This condition is specific to Linux EAL. In fact, it's not very useful even
> > > > there, because actual upper bound for `us` depends on current time.
> > > > No bounds are specified in API description at all.
> > > > Windows check would be different, but these considerations remain valid.
> > > >
> > > > Maybe it's alarm_autotest or API description that needs adjustments,
> > > > but not the implementation.
> > >
> > > i agree with your assessment. it's a bit silly to test a range
> > > constraint where the range is just some arbitrary range and not the real
> > > range. changing the implementation to calculate the "real" valid range
> > > isn't practical. i'm sure linux implementors would argue that catching
> > > some extremely out of range values is better than none?
> >
> > Why we care about overflow?
> > 1. It likely indicates a bug in the app.
> > 2. Alarm is can to be scheduled to some time in the past and fire
> > immediately, which means API did not do what it promises.
> > I see the only way to tell the interval is incorrect if it gives
> > (would give) time in the past when added to the current time.
> > But this is an implementation detail and does not need testing.
> > A separate patch can be submitted to change behavior for all OS.
> >
> > >
> > > i guess a correct test would would calculate the valid range and test
> > > against that but as per above the implementation won't pass the test.
> > >
> > > so we are left with
> > >
> > > * matching the range imposed in the linux implementation so we pass the
> > > test as is.
> >
> > It would be the worst thing one could do, defying the purpose of unit tests.
>
> agreed.
>
> >
> > > * don't run the test with the input data that exercises this bogus range
> > > constraint.
> > >
> > > i guess based on your comments you prefer the latter? so is our action
> > > here to submit a patch for the test that doesn't test this range
> > > conditionally on execenv windows?
> >
> > Yes, please remove this check from the test as it verifies no contract.
>
> we'll go with this.
>
> thanks
Thanks Dmitry and Tyler. Will address these in V2.
@@ -4,6 +4,7 @@
#include <stdatomic.h>
#include <stdbool.h>
+#include <inttypes.h>
#include <rte_alarm.h>
#include <rte_spinlock.h>
@@ -91,6 +92,22 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
LARGE_INTEGER deadline;
int ret;
+ /* Check if us is valid */
+ if (us < 1 || us >(UINT64_MAX - US_PER_S)) {
+ RTE_LOG(ERR, EAL, "Invalid us: %" PRIu64 "\n"
+ "Valid us range is 1 to (UINT64_MAX - US_PER_S)\n",
+ us);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ /* Check if callback is not NULL */
+ if (!cb_fn) {
+ RTE_LOG(ERR, EAL, "NULL callback\n");
+ ret = -EINVAL;
+ goto exit;
+ }
+
/* Calculate deadline ASAP, unit of measure = 100ns. */
GetSystemTimePreciseAsFileTime(&ft);
deadline.LowPart = ft.dwLowDateTime;
@@ -180,6 +197,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
bool executing;
removed = 0;
+
+ if (!cb_fn) {
+ RTE_LOG(ERR, EAL, "NULL callback\n");
+ return -EINVAL;
+ }
+
do {
executing = false;