On Tue, Nov 5, 2019 at 4:35 PM Brendan Higgins <brendanhiggins@xxxxxxxxxx> wrote: > > On Wed, Oct 30, 2019 at 11:59 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote: > > > > On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote: > > > On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins > > > <brendanhiggins@xxxxxxxxxx> wrote: > > > > > > > +config SECURITY_APPARMOR_TEST > > > > + bool "Build KUnit tests for policy_unpack.c" > > > > + default n > > > > New options already already default n, this can be left off. > > > > > > + depends on KUNIT && SECURITY_APPARMOR > > > > + help > > > > > > > select SECURITY_APPARMOR ? > > > > "select" doesn't enforce dependencies, so just a "depends ..." is > > correct. > > > > > > + KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE); > > > > + KUNIT_EXPECT_TRUE(test, > > > > + memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0); > > > I think this must be KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);, > > > otherwise there could be a buffer overflow in memcmp. All tests that > > > follow such pattern > > > > Agreed. > > > > > are suspect. Also, not sure about your stylistic preference for > > > KUNIT_EXPECT_TRUE(test, > > > memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0); > > > vs > > > KUNIT_EXPECT_EQ(test, > > > 0, > > > memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE)); > > > > I like == 0. > > Oh, I almost missed this. I think the *_EQ(...) is better than the > *_TRUE(...) because the EQ is able to provide more debug information > if the test fails (otherwise there would really be no point in > providing all these variants). > > Any objections? > > Thanks for the catch Iurii! Wait, nevermind. Either way is fine because memcmp probably won't show terribly interesting information in the non-zero case. I'll just leave it the way Mike wrote it. Sorry!
