* [PATCH] kunit: tool: add null pointer check
@ 2022-03-29 10:39 cgel.zte
2022-03-29 19:29 ` Daniel Latypov
0 siblings, 1 reply; 4+ messages in thread
From: cgel.zte @ 2022-03-29 10:39 UTC (permalink / raw)
To: brendanhiggins
Cc: linux-kselftest, kunit-dev, linux-kernel, Lv Ruyi, Zeal Robot
From: Lv Ruyi <lv.ruyi@zte.com.cn>
kmalloc and kcalloc is a memory allocation function which can return NULL
when some internal memory errors happen. Add null pointer check to avoid
dereferencing null pointer.
Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn>
---
lib/kunit/executor.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 22640c9ee819..be21d0451367 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -71,9 +71,13 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob)
/* Use memcpy to workaround copy->name being const. */
copy = kmalloc(sizeof(*copy), GFP_KERNEL);
+ if (!copy)
+ return NULL;
memcpy(copy, suite, sizeof(*copy));
filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL);
+ if (!filtered)
+ return NULL;
n = 0;
kunit_suite_for_each_test_case(suite, test_case) {
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: tool: add null pointer check
2022-03-29 10:39 [PATCH] kunit: tool: add null pointer check cgel.zte
@ 2022-03-29 19:29 ` Daniel Latypov
2022-03-29 21:32 ` Brendan Higgins
2022-03-29 22:54 ` Daniel Latypov
0 siblings, 2 replies; 4+ messages in thread
From: Daniel Latypov @ 2022-03-29 19:29 UTC (permalink / raw)
To: cgel.zte
Cc: brendanhiggins, linux-kselftest, kunit-dev, linux-kernel,
Lv Ruyi, Zeal Robot
On Tue, Mar 29, 2022 at 5:39 AM <cgel.zte@gmail.com> wrote:
>
> From: Lv Ruyi <lv.ruyi@zte.com.cn>
>
> kmalloc and kcalloc is a memory allocation function which can return NULL
> when some internal memory errors happen. Add null pointer check to avoid
> dereferencing null pointer.
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn>
> ---
> lib/kunit/executor.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 22640c9ee819..be21d0451367 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -71,9 +71,13 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob)
>
> /* Use memcpy to workaround copy->name being const. */
> copy = kmalloc(sizeof(*copy), GFP_KERNEL);
> + if (!copy)
> + return NULL;
While this is technically correct to check, in this context it's less clear.
If we can't allocate this memory, we likely can't run any subsequent
tests, either because the test cases will want to allocate some memory
and/or KUnit will need to allocate some for internal bookkeeping.
The existing code (and by extension this patch) "handles" OOM
situations by silently dropping test suites/cases.
So I sort of intentionally figured we should let it crash early in
this case since that's probably more debuggable.
This code does check for NULL returns earlier on in the call chain, i.e.
first in kunit_filter_suites()
158 copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL);
159 filtered.start = copy;
160 if (!copy) { /* won't be able to run anything, return
an empty set */
161 filtered.end = copy;
162 return filtered;
163 }
and second in kunit_filter_subsuite()
107 filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL);
108 if (!filtered)
109 return NULL;
The first kmalloc_array() is our first allocation in this file.
If we can't handle that, then things are really going wrong, and I
assumed there'd be plenty of debug messages in dmesg, so silently
returning is probably fine.
The second one also felt similar.
So I think that
* it's highly unlikely that we pass those checks and fail on these new
ones (we're not allocating much)
* if we do fail, this is now harder to debug since it's partially
running tests, partially not
Should we instead rework the code to more clearly signal allocation
errors instead of overloading NULL to mean "no matches or error?"
Or maybe just adding some pr_err() calls is sufficient.
> memcpy(copy, suite, sizeof(*copy));
>
> filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL);
> + if (!filtered)
> + return NULL;
>
> n = 0;
> kunit_suite_for_each_test_case(suite, test_case) {
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: tool: add null pointer check
2022-03-29 19:29 ` Daniel Latypov
@ 2022-03-29 21:32 ` Brendan Higgins
2022-03-29 22:54 ` Daniel Latypov
1 sibling, 0 replies; 4+ messages in thread
From: Brendan Higgins @ 2022-03-29 21:32 UTC (permalink / raw)
To: Daniel Latypov
Cc: cgel.zte, linux-kselftest, kunit-dev, linux-kernel, Lv Ruyi, Zeal Robot
On Tue, Mar 29, 2022 at 3:29 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Tue, Mar 29, 2022 at 5:39 AM <cgel.zte@gmail.com> wrote:
> >
> > From: Lv Ruyi <lv.ruyi@zte.com.cn>
> >
> > kmalloc and kcalloc is a memory allocation function which can return NULL
> > when some internal memory errors happen. Add null pointer check to avoid
> > dereferencing null pointer.
> >
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn>
> > ---
> > lib/kunit/executor.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 22640c9ee819..be21d0451367 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -71,9 +71,13 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob)
> >
> > /* Use memcpy to workaround copy->name being const. */
> > copy = kmalloc(sizeof(*copy), GFP_KERNEL);
> > + if (!copy)
> > + return NULL;
>
> While this is technically correct to check, in this context it's less clear.
> If we can't allocate this memory, we likely can't run any subsequent
> tests, either because the test cases will want to allocate some memory
> and/or KUnit will need to allocate some for internal bookkeeping.
>
> The existing code (and by extension this patch) "handles" OOM
> situations by silently dropping test suites/cases.
> So I sort of intentionally figured we should let it crash early in
> this case since that's probably more debuggable.
>
> This code does check for NULL returns earlier on in the call chain, i.e.
>
> first in kunit_filter_suites()
> 158 copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL);
> 159 filtered.start = copy;
> 160 if (!copy) { /* won't be able to run anything, return
> an empty set */
> 161 filtered.end = copy;
> 162 return filtered;
> 163 }
>
> and second in kunit_filter_subsuite()
> 107 filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL);
> 108 if (!filtered)
> 109 return NULL;
>
> The first kmalloc_array() is our first allocation in this file.
> If we can't handle that, then things are really going wrong, and I
> assumed there'd be plenty of debug messages in dmesg, so silently
> returning is probably fine.
> The second one also felt similar.
>
> So I think that
> * it's highly unlikely that we pass those checks and fail on these new
> ones (we're not allocating much)
> * if we do fail, this is now harder to debug since it's partially
> running tests, partially not
>
> Should we instead rework the code to more clearly signal allocation
> errors instead of overloading NULL to mean "no matches or error?"
> Or maybe just adding some pr_err() calls is sufficient.
I think we should either return an err ptr, or log something (maybe both).
But yeah, I agree with you Daniel, I don't like overloading NULL.
> > memcpy(copy, suite, sizeof(*copy));
> >
> > filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL);
> > + if (!filtered)
> > + return NULL;
> >
> > n = 0;
> > kunit_suite_for_each_test_case(suite, test_case) {
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: tool: add null pointer check
2022-03-29 19:29 ` Daniel Latypov
2022-03-29 21:32 ` Brendan Higgins
@ 2022-03-29 22:54 ` Daniel Latypov
1 sibling, 0 replies; 4+ messages in thread
From: Daniel Latypov @ 2022-03-29 22:54 UTC (permalink / raw)
To: cgel.zte
Cc: brendanhiggins, linux-kselftest, kunit-dev, linux-kernel,
Lv Ruyi, Zeal Robot
On Tue, Mar 29, 2022 at 2:29 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Tue, Mar 29, 2022 at 5:39 AM <cgel.zte@gmail.com> wrote:
> >
> > From: Lv Ruyi <lv.ruyi@zte.com.cn>
> >
> > kmalloc and kcalloc is a memory allocation function which can return NULL
> > when some internal memory errors happen. Add null pointer check to avoid
> > dereferencing null pointer.
> >
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn>
> > ---
> > lib/kunit/executor.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 22640c9ee819..be21d0451367 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -71,9 +71,13 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob)
> >
> > /* Use memcpy to workaround copy->name being const. */
> > copy = kmalloc(sizeof(*copy), GFP_KERNEL);
> > + if (!copy)
> > + return NULL;
>
> While this is technically correct to check, in this context it's less clear.
> If we can't allocate this memory, we likely can't run any subsequent
> tests, either because the test cases will want to allocate some memory
> and/or KUnit will need to allocate some for internal bookkeeping.
>
> The existing code (and by extension this patch) "handles" OOM
> situations by silently dropping test suites/cases.
> So I sort of intentionally figured we should let it crash early in
> this case since that's probably more debuggable.
>
> This code does check for NULL returns earlier on in the call chain, i.e.
>
> first in kunit_filter_suites()
> 158 copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL);
> 159 filtered.start = copy;
> 160 if (!copy) { /* won't be able to run anything, return
> an empty set */
> 161 filtered.end = copy;
> 162 return filtered;
> 163 }
>
> and second in kunit_filter_subsuite()
> 107 filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL);
> 108 if (!filtered)
> 109 return NULL;
>
> The first kmalloc_array() is our first allocation in this file.
> If we can't handle that, then things are really going wrong, and I
> assumed there'd be plenty of debug messages in dmesg, so silently
> returning is probably fine.
> The second one also felt similar.
>
> So I think that
> * it's highly unlikely that we pass those checks and fail on these new
> ones (we're not allocating much)
> * if we do fail, this is now harder to debug since it's partially
> running tests, partially not
>
> Should we instead rework the code to more clearly signal allocation
> errors instead of overloading NULL to mean "no matches or error?"
More concretely, I'm thinking something like this:
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 22640c9ee819..a5c29a32a33a 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -71,9 +71,13 @@ kunit_filter_tests(struct kunit_suite *const suite,
const char *test_glob)
/* Use memcpy to workaround copy->name being const. */
copy = kmalloc(sizeof(*copy), GFP_KERNEL);
+ if (!copy)
+ return ERR_PTR(-ENOMEM);
memcpy(copy, suite, sizeof(*copy));
filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL);
+ if (!filtered)
+ return ERR_PTR(-ENOMEM);
n = 0;
kunit_suite_for_each_test_case(suite, test_case) {
@@ -106,14 +110,16 @@ kunit_filter_subsuite(struct kunit_suite * const
* const subsuite,
filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL);
if (!filtered)
- return NULL;
+ return ERR_PTR(-ENOMEM);
n = 0;
for (i = 0; subsuite[i] != NULL; ++i) {
if (!glob_match(filter->suite_glob, subsuite[i]->name))
continue;
filtered_suite = kunit_filter_tests(subsuite[i],
filter->test_glob);
- if (filtered_suite)
+ if (IS_ERR(filtered_suite))
+ return ERR_CAST(filtered_suite);
+ else if (filtered_suite)
filtered[n++] = filtered_suite;
}
filtered[n] = NULL;
@@ -166,6 +172,8 @@ static struct suite_set kunit_filter_suites(const
struct suite_set *suite_set,
for (i = 0; i < max; ++i) {
filtered_subsuite =
kunit_filter_subsuite(suite_set->start[i], &filter);
+ if (IS_ERR(filtered_subsuite))
+ return filtered; // TODO: how do we signal this?
if (filtered_subsuite)
*copy++ = filtered_subsuite;
}
> Or maybe just adding some pr_err() calls is sufficient.
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-29 22:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-29 10:39 [PATCH] kunit: tool: add null pointer check cgel.zte
2022-03-29 19:29 ` Daniel Latypov
2022-03-29 21:32 ` Brendan Higgins
2022-03-29 22:54 ` Daniel Latypov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.