Messages in this thread | | | From | Brendan Higgins <> | Date | Tue, 5 Nov 2019 16:37:16 -0800 | Subject | Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack |
| |
On Tue, Nov 5, 2019 at 4:35 PM Brendan Higgins <brendanhiggins@google.com> wrote: > > On Wed, Oct 30, 2019 at 11:59 AM Kees Cook <keescook@chromium.org> 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@google.com> 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!
| |