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
[off-list ref] wrote:

On Wed, Oct 30, 2019 at 11:59 AM Kees Cook [off-list ref] wrote:

quoted

On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:

quoted

On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins
[off-list ref] wrote:

quoted

+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.

quoted

quoted

+       depends on KUNIT && SECURITY_APPARMOR
+       help

select SECURITY_APPARMOR ?

"select" doesn't enforce dependencies, so just a "depends ..." is
correct.

quoted

quoted

+       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.

quoted

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!