| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 14 May 2024 15:55:20 -0400
From: Rae Moar <rmoar@...gle.com>
To: Ivan Orlov <ivan.orlov0322@...il.com>
Cc: brendan.higgins@...ux.dev, davidgow@...gle.com,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
kunit-dev@...glegroups.com, skhan@...uxfoundation.org
Subject: Re: [PATCH v3] kunit: Cover 'assert.c' with tests
On Tue, May 14, 2024 at 10:31 AM Ivan Orlov <ivan.orlov0322@...il.com> wrote:
>
> On 5/14/24 01:17, Rae Moar wrote:
> > On Thu, May 9, 2024 at 5:05 AM Ivan Orlov <ivan.orlov0322@...ilcom> wrote:
> >>
> >> There are multiple assertion formatting functions in the `assert.c`
> >> file, which are not covered with tests yet. Implement the KUnit test
> >> for these functions.
> >>
> >> The test consists of 11 test cases for the following functions:
> >>
> >> 1) 'is_literal'
> >> 2) 'is_str_literal'
> >> 3) 'kunit_assert_prologue', test case for multiple assert types
> >> 4) 'kunit_assert_print_msg'
> >> 5) 'kunit_unary_assert_format'
> >> 6) 'kunit_ptr_not_err_assert_format'
> >> 7) 'kunit_binary_assert_format'
> >> 8) 'kunit_binary_ptr_assert_format'
> >> 9) 'kunit_binary_str_assert_format'
> >> 10) 'kunit_assert_hexdump'
> >> 11) 'kunit_mem_assert_format'
> >>
> >> The test aims at maximizing the branch coverage for the assertion
> >> formatting functions.
> >>
> >> As you can see, it covers some of the static helper functions as
> >> well, so mark the static functions in `assert.c` as 'VISIBLE_IF_KUNIT'
> >> and conditionally export them with EXPORT_SYMBOL_IF_KUNIT. Add the
> >> corresponding definitions to `assert.h`.
> >>
> >> Build the assert test when CONFIG_KUNIT_TEST is enabled, similar to
> >> how it is done for the string stream test.
> >
> > Hello!
> >
> > This looks great to me! Thanks for all your work on this! There is
> > just one comment I have below. Once that is fixed up, I am happy to
> > add a reviewed-by.
> >
> > Thanks!
> > -Rae
> >
> >>
> >> Signed-off-by: Ivan Orlov <ivan.orlov0322@...il.com>
> >> ---
> >> V1 -> V2:
> >> - Check the output from the string stream for containing the key parts
> >> instead of comparing the results with expected strings char by char, as
> >> it was suggested by Rae Moar <rmoar@...gle.com>. Define two macros to
> >> make it possible (ASSERT_TEST_EXPECT_CONTAIN and
> >> ASSERT_TEST_EXPECT_NCONTAIN).
> >> - Mark the static functions in `assert.c` as VISIBLE_IF_KUNIT and export
> >> them conditionally if kunit is enabled instead of including the
> >> `assert_test.c` file in the end of `assert.c`. This way we will decouple
> >> the test from the implementation (SUT).
> >> - Update the kunit_assert_hexdump test: now it checks for presense of
> >> the brackets '<>' around the non-matching bytes, instead of comparing
> >> the kunit_assert_hexdump output char by char.
> >> V2 -> V3:
> >> - Make test case array and test suite definitions static
> >> - Change the condition in `assert.h`: we should declare VISIBLE_IF_KUNIT
> >> functions in the header file when CONFIG_KUNIT is enabled, not
> >> CONFIG_KUNIT_TEST. Otherwise, if CONFIG_KUNIT_TEST is disabled,
> >> VISIBLE_IF_KUNIT functions in the `assert.c` are not static, and
> >> prototypes for them can't be found.
> >> - Add MODULE_LICENSE and MODULE_DESCRIPTION macros
> >>
> >> include/kunit/assert.h | 11 ++
> >> lib/kunit/Makefile | 1 +
> >> lib/kunit/assert.c | 24 ++-
> >> lib/kunit/assert_test.c | 391 ++++++++++++++++++++++++++++++++++++++++
> >> 4 files changed, 419 insertions(+), 8 deletions(-)
> >> create mode 100644 lib/kunit/assert_test.c
> >>
> >> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> >> index 24c2b9fa61e8..7e7490a74b13 100644
> >> --- a/include/kunit/assert.h
> >> +++ b/include/kunit/assert.h
> >> @@ -218,4 +218,15 @@ void kunit_mem_assert_format(const struct kunit_assert *assert,
> >> const struct va_format *message,
> >> struct string_stream *stream);
> >>
> >> +#if IS_ENABLED(CONFIG_KUNIT)
> >> +void kunit_assert_print_msg(const struct va_format *message,
> >> + struct string_stream *stream);
> >> +bool is_literal(const char *text, long long value);
> >> +bool is_str_literal(const char *text, const char *value);
> >> +void kunit_assert_hexdump(struct string_stream *stream,
> >> + const void *buf,
> >> + const void *compared_buf,
> >> + const size_t len);
> >> +#endif
> >> +
> >> #endif /* _KUNIT_ASSERT_H */
> >> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> >> index 309659a32a78..be7c9903936f 100644
> >> --- a/lib/kunit/Makefile
> >> +++ b/lib/kunit/Makefile
> >> @@ -18,6 +18,7 @@ endif
> >> obj-y += hooks.o
> >>
> >> obj-$(CONFIG_KUNIT_TEST) += kunit-test.o
> >> +obj-$(CONFIG_KUNIT_TEST) += assert_test.o
> >>
> >> # string-stream-test compiles built-in only.
> >> ifeq ($(CONFIG_KUNIT_TEST),y)
> >> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> >> index dd1d633d0fe2..382eb409d34b 100644
> >> --- a/lib/kunit/assert.c
> >> +++ b/lib/kunit/assert.c
> >> @@ -7,6 +7,7 @@
> >> */
> >> #include <kunit/assert.h>
> >> #include <kunit/test.h>
> >> +#include <kunit/visibility.h>
> >>
> >> #include "string-stream.h"
> >>
> >> @@ -30,12 +31,14 @@ void kunit_assert_prologue(const struct kunit_loc *loc,
> >> }
> >> EXPORT_SYMBOL_GPL(kunit_assert_prologue);
> >>
> >> -static void kunit_assert_print_msg(const struct va_format *message,
> >> - struct string_stream *stream)
> >> +VISIBLE_IF_KUNIT
> >> +void kunit_assert_print_msg(const struct va_format *message,
> >> + struct string_stream *stream)
> >> {
> >> if (message->fmt)
> >> string_stream_add(stream, "\n%pV", message);
> >> }
> >> +EXPORT_SYMBOL_IF_KUNIT(kunit_assert_print_msg);
> >>
> >> void kunit_fail_assert_format(const struct kunit_assert *assert,
> >> const struct va_format *message,
> >> @@ -89,7 +92,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
> >> EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format);
> >>
> >> /* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */
> >> -static bool is_literal(const char *text, long long value)
> >> +VISIBLE_IF_KUNIT bool is_literal(const char *text, long long value)
> >> {
> >> char *buffer;
> >> int len;
> >> @@ -110,6 +113,7 @@ static bool is_literal(const char *text, long long value)
> >>
> >> return ret;
> >> }
> >> +EXPORT_SYMBOL_IF_KUNIT(is_literal);
> >>
> >> void kunit_binary_assert_format(const struct kunit_assert *assert,
> >> const struct va_format *message,
> >> @@ -166,7 +170,7 @@ EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format);
> >> /* Checks if KUNIT_EXPECT_STREQ() args were string literals.
> >> * Note: `text` will have ""s where as `value` will not.
> >> */
> >> -static bool is_str_literal(const char *text, const char *value)
> >> +VISIBLE_IF_KUNIT bool is_str_literal(const char *text, const char *value)
> >> {
> >> int len;
> >>
> >> @@ -178,6 +182,7 @@ static bool is_str_literal(const char *text, const char *value)
> >>
> >> return strncmp(text + 1, value, len - 2) == 0;
> >> }
> >> +EXPORT_SYMBOL_IF_KUNIT(is_str_literal);
> >>
> >> void kunit_binary_str_assert_format(const struct kunit_assert *assert,
> >> const struct va_format *message,
> >> @@ -208,10 +213,11 @@ EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format);
> >> /* Adds a hexdump of a buffer to a string_stream comparing it with
> >> * a second buffer. The different bytes are marked with <>.
> >> */
> >> -static void kunit_assert_hexdump(struct string_stream *stream,
> >> - const void *buf,
> >> - const void *compared_buf,
> >> - const size_t len)
> >> +VISIBLE_IF_KUNIT
> >> +void kunit_assert_hexdump(struct string_stream *stream,
> >> + const void *buf,
> >> + const void *compared_buf,
> >> + const size_t len)
> >> {
> >> size_t i;
> >> const u8 *buf1 = buf;
> >> @@ -229,6 +235,7 @@ static void kunit_assert_hexdump(struct string_stream *stream,
> >> string_stream_add(stream, " %02x ", buf1[i]);
> >> }
> >> }
> >> +EXPORT_SYMBOL_IF_KUNIT(kunit_assert_hexdump);
> >>
> >> void kunit_mem_assert_format(const struct kunit_assert *assert,
> >> const struct va_format *message,
> >> @@ -269,4 +276,5 @@ void kunit_mem_assert_format(const struct kunit_assert *assert,
> >> kunit_assert_print_msg(message, stream);
> >> }
> >> }
> >> +
> >> EXPORT_SYMBOL_GPL(kunit_mem_assert_format);
> >> diff --git a/lib/kunit/assert_test.c b/lib/kunit/assert_test.c
> >> new file mode 100644
> >> index 000000000000..1347a964204b
> >> --- /dev/null
> >> +++ b/lib/kunit/assert_test.c
> >> @@ -0,0 +1,391 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * KUnit test for the assertion formatting functions.
> >> + * Author: Ivan Orlov <ivan.orlov0322@...il.com>
> >> + */
> >> +#include <kunit/test.h>
> >> +#include "string-stream.h"
> >> +
> >> +#define TEST_PTR_EXPECTED_BUF_SIZE 32
> >> +#define HEXDUMP_TEST_BUF_LEN 5
> >> +#define ASSERT_TEST_EXPECT_CONTAIN(test, str, substr) KUNIT_EXPECT_TRUE(test, strstr(str, substr))
> >> +#define ASSERT_TEST_EXPECT_NCONTAIN(test, str, substr) KUNIT_EXPECT_FALSE(test, strstr(str, substr))
> >> +
> >> +static void kunit_test_is_literal(struct kunit *test)
> >> +{
> >> + KUNIT_EXPECT_TRUE(test, is_literal("5", 5));
> >> + KUNIT_EXPECT_TRUE(test, is_literal("0", 0));
> >> + KUNIT_EXPECT_TRUE(test, is_literal("1234567890", 1234567890));
> >> + KUNIT_EXPECT_TRUE(test, is_literal("-1234567890", -1234567890));
> >> + KUNIT_EXPECT_FALSE(test, is_literal("05", 5));
> >> + KUNIT_EXPECT_FALSE(test, is_literal("", 0));
> >> + KUNIT_EXPECT_FALSE(test, is_literal("-0", 0));
> >> + KUNIT_EXPECT_FALSE(test, is_literal("12#45", 1245));
> >> +}
> >> +
> >> +static void kunit_test_is_str_literal(struct kunit *test)
> >> +{
> >> + KUNIT_EXPECT_TRUE(test, is_str_literal("\"Hello, World!\"", "Hello, World!"));
> >> + KUNIT_EXPECT_TRUE(test, is_str_literal("\"\"", ""));
> >> + KUNIT_EXPECT_TRUE(test, is_str_literal("\"\"\"", "\""));
> >> + KUNIT_EXPECT_FALSE(test, is_str_literal("", ""));
> >> + KUNIT_EXPECT_FALSE(test, is_str_literal("\"", "\""));
> >> + KUNIT_EXPECT_FALSE(test, is_str_literal("\"Abacaba", "Abacaba"));
> >> + KUNIT_EXPECT_FALSE(test, is_str_literal("Abacaba\"", "Abacaba"));
> >> + KUNIT_EXPECT_FALSE(test, is_str_literal("\"Abacaba\"", "\"Abacaba\""));
> >> +}
> >> +
> >> +KUNIT_DEFINE_ACTION_WRAPPER(kfree_wrapper, kfree, const void *);
> >> +
> >> +/* this function is used to get a "char *" string from the string stream and defer its cleanup */
> >> +static char *get_str_from_stream(struct kunit *test, struct string_stream *stream)
> >> +{
> >> + char *str = string_stream_get_string(stream);
> >> +
> >
> > When trying to make the kernel with this test loaded in, I am getting
> > an error that string_stream_get_string, string_stream_clear, and
> > kunit_alloc_string_stream are undefined.
> >
> > So either these three methods will have to be exported using
> > EXPORT_SYMBOL_KUNIT or this test cannot be loaded and run as a module.
> >
> > But once this is fixed up this should be good to go.
>
> Hi Rae,
>
> Thank you so much for the review.
>
> At the moment, I believe the best approach would be to make this test
> depend on CONFIG_KUNIT_TEST=y (as it is done for string-stream-test).
>
> However, I assume that every (standalone) test should be able to run as
> a module, and I'd like to add EXPORT_SYMBOL_IF_KUNIT to all of the
> non-static string-stream functions in a separate patch series. It will
> require updating string-stream-test.c as well (adding MODULE_IMPORT_NS).
> What do you think?
>
> Thank you once again,
> --
> Kind regards,
> Ivan Orlov
Hello!
This sounds like a great approach! Happy to review the new patch
series when it comes in.
Thanks,
Rae
>
Powered by blists - more mailing lists