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 nNew options already already default n, this can be left off.
depends on KUNIT && SECURITY_APPARMORhelpselect 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!