| Message ID | 20240710000220.work.852-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kunit: executor: Simplify string allocation handling | expand |
On Wed, 10 Jul 2024 at 08:02, Kees Cook <kees@kernel.org> wrote: > > The alloc/copy code pattern is better consolidated to single kstrdup (and > kstrndup) calls instead. This gets rid of deprecated[1] strncpy() uses as > well. Replace one other strncpy() use with the more idiomatic strscpy(). > > Link: https://github.com/KSPP/linux/issues/90 [1] > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: Brendan Higgins <brendan.higgins@linux.dev> > Cc: David Gow <davidgow@google.com> > Cc: Rae Moar <rmoar@google.com> > Cc: linux-kselftest@vger.kernel.org > Cc: kunit-dev@googlegroups.com > --- Looks good apart from the strscpy() change, which is broken by the (char *) cast. Using the 3-argument version worked here. With the strscpy() fixed, this is: Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > lib/kunit/executor.c | 12 +++--------- > lib/kunit/executor_test.c | 2 +- > 2 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > index 70b9a43cd257..34b7b6833df3 100644 > --- a/lib/kunit/executor.c > +++ b/lib/kunit/executor.c > @@ -70,32 +70,26 @@ struct kunit_glob_filter { > static int kunit_parse_glob_filter(struct kunit_glob_filter *parsed, > const char *filter_glob) > { > - const int len = strlen(filter_glob); > const char *period = strchr(filter_glob, '.'); > > if (!period) { > - parsed->suite_glob = kzalloc(len + 1, GFP_KERNEL); > + parsed->suite_glob = kstrdup(filter_glob, GFP_KERNEL); > if (!parsed->suite_glob) > return -ENOMEM; > - > parsed->test_glob = NULL; > - strcpy(parsed->suite_glob, filter_glob); > return 0; > } > > - parsed->suite_glob = kzalloc(period - filter_glob + 1, GFP_KERNEL); > + parsed->suite_glob = kstrndup(filter_glob, period - filter_glob, GFP_KERNEL); > if (!parsed->suite_glob) > return -ENOMEM; > > - parsed->test_glob = kzalloc(len - (period - filter_glob) + 1, GFP_KERNEL); > + parsed->test_glob = kstrdup(period + 1, GFP_KERNEL); > if (!parsed->test_glob) { > kfree(parsed->suite_glob); > return -ENOMEM; > } > > - strncpy(parsed->suite_glob, filter_glob, period - filter_glob); > - strncpy(parsed->test_glob, period + 1, len - (period - filter_glob)); > - > return 0; > } > > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > index 3f7f967e3688..7191be9c4f9b 100644 > --- a/lib/kunit/executor_test.c > +++ b/lib/kunit/executor_test.c > @@ -286,7 +286,7 @@ static struct kunit_suite *alloc_fake_suite(struct kunit *test, > > /* We normally never expect to allocate suites, hence the non-const cast. */ > suite = kunit_kzalloc(test, sizeof(*suite), GFP_KERNEL); > - strncpy((char *)suite->name, suite_name, sizeof(suite->name) - 1); > + strscpy((char *)suite->name, suite_name); This is broken: we still need to pass the length of suite->name. The (char *) cast, which is necessary to remove the 'cosnt' qualifier, stops the strscpy() macro from treating suite->name as an array. > suite->test_cases = test_cases; > > return suite; > -- > 2.34.1 > >
On Thu, Jul 11, 2024 at 01:39:15PM +0800, David Gow wrote: > On Wed, 10 Jul 2024 at 08:02, Kees Cook <kees@kernel.org> wrote: > > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > > index 3f7f967e3688..7191be9c4f9b 100644 > > --- a/lib/kunit/executor_test.c > > +++ b/lib/kunit/executor_test.c > > @@ -286,7 +286,7 @@ static struct kunit_suite *alloc_fake_suite(struct kunit *test, > > > > /* We normally never expect to allocate suites, hence the non-const cast. */ > > suite = kunit_kzalloc(test, sizeof(*suite), GFP_KERNEL); > > - strncpy((char *)suite->name, suite_name, sizeof(suite->name) - 1); > > + strscpy((char *)suite->name, suite_name); > > This is broken: we still need to pass the length of suite->name. The > (char *) cast, which is necessary to remove the 'cosnt' qualifier, > stops the strscpy() macro from treating suite->name as an array. Ah! Thanks for catching that. I do build tests with "allmodconfig", and I saw the #include for executor_test.c, but didn't notice it was for _builtin_ only...
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 70b9a43cd257..34b7b6833df3 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -70,32 +70,26 @@ struct kunit_glob_filter { static int kunit_parse_glob_filter(struct kunit_glob_filter *parsed, const char *filter_glob) { - const int len = strlen(filter_glob); const char *period = strchr(filter_glob, '.'); if (!period) { - parsed->suite_glob = kzalloc(len + 1, GFP_KERNEL); + parsed->suite_glob = kstrdup(filter_glob, GFP_KERNEL); if (!parsed->suite_glob) return -ENOMEM; - parsed->test_glob = NULL; - strcpy(parsed->suite_glob, filter_glob); return 0; } - parsed->suite_glob = kzalloc(period - filter_glob + 1, GFP_KERNEL); + parsed->suite_glob = kstrndup(filter_glob, period - filter_glob, GFP_KERNEL); if (!parsed->suite_glob) return -ENOMEM; - parsed->test_glob = kzalloc(len - (period - filter_glob) + 1, GFP_KERNEL); + parsed->test_glob = kstrdup(period + 1, GFP_KERNEL); if (!parsed->test_glob) { kfree(parsed->suite_glob); return -ENOMEM; } - strncpy(parsed->suite_glob, filter_glob, period - filter_glob); - strncpy(parsed->test_glob, period + 1, len - (period - filter_glob)); - return 0; } diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c index 3f7f967e3688..7191be9c4f9b 100644 --- a/lib/kunit/executor_test.c +++ b/lib/kunit/executor_test.c @@ -286,7 +286,7 @@ static struct kunit_suite *alloc_fake_suite(struct kunit *test, /* We normally never expect to allocate suites, hence the non-const cast. */ suite = kunit_kzalloc(test, sizeof(*suite), GFP_KERNEL); - strncpy((char *)suite->name, suite_name, sizeof(suite->name) - 1); + strscpy((char *)suite->name, suite_name); suite->test_cases = test_cases; return suite;
The alloc/copy code pattern is better consolidated to single kstrdup (and kstrndup) calls instead. This gets rid of deprecated[1] strncpy() uses as well. Replace one other strncpy() use with the more idiomatic strscpy(). Link: https://github.com/KSPP/linux/issues/90 [1] Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: Brendan Higgins <brendan.higgins@linux.dev> Cc: David Gow <davidgow@google.com> Cc: Rae Moar <rmoar@google.com> Cc: linux-kselftest@vger.kernel.org Cc: kunit-dev@googlegroups.com --- lib/kunit/executor.c | 12 +++--------- lib/kunit/executor_test.c | 2 +- 2 files changed, 4 insertions(+), 10 deletions(-)