Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56813 )
Change subject: tests: Add lib/cbfs-lookup-test test case ......................................................................
Patch Set 7:
(28 comments)
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56813/comment/d55d4c3a_0e98f0e8 PS7, Line 222: cbmem_top Unused now?
https://review.coreboot.org/c/coreboot/+/56813/comment/cc1e3066_99b74846 PS7, Line 226: CONFIG_CBFS_VERIFICATION=0 This is the default anyway (and should remain so for the foreseeable future).
File tests/lib/cbfs-lookup-test.c:
https://review.coreboot.org/c/coreboot/+/56813/comment/7e23ce93_20e7169a PS4, Line 115: BE32(CBFS_FILE_ATTR_TAG_UNUSED),
So what about CBFS_FILE_ATTR_TAG_UNUSED and CBFS_FILE_ATTR_TAG_UNUSED2? They are here only for cbfst […]
Well, as the name says they're unused. They're just defined in the enum to block that value off so that nobody accidentally defines a new attribute with value 0x00 or 0xff (which for various reasons could be a bad idea). They are never valid in a real CBFS image
https://review.coreboot.org/c/coreboot/+/56813/comment/e981031c_6a09756c PS4, Line 139: },
Now all tests are executed for aligned and unaligned buffer (in terms of address alignment, not CBFS_ALIGNMENT)
Right, but here I actually meant alignment in relation to the start of the rdev (cbfs_walk() should skip headers not on an alignment boundary, test should verify that behavior).
I think, that these cases are covered by those:
Well yes, it's kinda the same thing, but I think it might be worth having both (one where it exceeds the available size by a little bit, and one where it uses a number very close to UINT32_MAX). I could imagine failure cases where the latter causes some sort of overflow or is incorrectly interpreted as a signed value somewhere.
https://review.coreboot.org/c/coreboot/+/56813/comment/823fc2e4_df1d32a4 PS4, Line 148: __aligned(CBFS_ALIGNMENT);
Done. However, I see that CBFS "aligns" files by CBFS_ALIGNMENT, but does not use ALIGN_UP/DOWN. […]
Sorry, not quite sure what you mean. Are you talking about where the image is parsed, or where it is generated? The parser uses ALIGN_UP (read_next_header() in src/commonlib/bsd/cbfs_private.c). cbfstool also respects the alignment, although it's a bit more complicated to follow why... cbfstool never really "creates" a CBFS file header in empty space, because all empty space is already covered by CBFS_TYPE_DELETED file entries. So all cbfstool ever does is split CBFS_TYPE_DELETED files into multiple smaller entries, and aligning those is handled by cbfs_add_entry_at() and cbfs_find_next_entry().
https://review.coreboot.org/c/coreboot/+/56813/comment/8211dede_8213b900 PS4, Line 240: cbd.mcache_size = TEST_MCACHE_SIZE;
Each test calls cbfs_init_boot_device() after constructing CBFS. […]
Okay, for this test sounds fine. But eventually we should also have a separate mcache test that tests all the edge cases for mcache_build specifically.
File tests/lib/cbfs-lookup-test.c:
https://review.coreboot.org/c/coreboot/+/56813/comment/4519ca27_83ad32ed PS7, Line 35: BE64 nit: as long as it's test data it doesn't really matter, but be aware that while CBFS header fields and attributes are always big-endian, the file *contents* have no such convention and are usually just in host byte order of the stage that accesses them. (And cbfstool add-integer actually seems to explicitly encode numbers as little-endian for some reason, as far as I can tell.)
https://review.coreboot.org/c/coreboot/+/56813/comment/73d0f834_f3e4e92c PS7, Line 80: break Should this not also assert()? (Could pull this assert() out below the if-else block, I think...)
https://review.coreboot.org/c/coreboot/+/56813/comment/4384c2ad_4efdea25 PS7, Line 90: if (offset % CBFS_ALIGNMENT != 0) nit: technically don't need to check first, ALIGN_UP() will do the right thing either way.
https://review.coreboot.org/c/coreboot/+/56813/comment/a76dc96e_47a16bb7 PS7, Line 120: #if CONFIG(NO_CBFS_MCACHE) nit: Should be no need to only compile one of the two? Should be okay if the other one just doesn't get called. In fact, defining it (with a call to mock_type()) when we don't expect it to get called serves to verify that it actually doesn't get called.
https://review.coreboot.org/c/coreboot/+/56813/comment/68735e0d_0d41de7a PS7, Line 154: { nit: might as well throw in an assert_ptr_equal(mp, _cbfs_cache) to both of these?
https://review.coreboot.org/c/coreboot/+/56813/comment/dd9fd9a5_71165b19 PS7, Line 185: size_t sz; nit: I'd maybe call these cbfs_buf and cbfs_size or img_buf and img_size, to make it more obvious in the code below what buffer exactly this represents (because things are getting pretty complicated down there).
https://review.coreboot.org/c/coreboot/+/56813/comment/78eac6d1_8d5b1d14 PS7, Line 198: s->sz = sizeof(aligned_cbfs_buffer); Maybe zero out s->ex just in case?
https://review.coreboot.org/c/coreboot/+/56813/comment/ea085dc0_c70d3121 PS7, Line 248: #define expect_lookup_result(res) \ nit: can't this just be a function?
https://review.coreboot.org/c/coreboot/+/56813/comment/00b9885a_7e8f476f PS7, Line 439: &s->buf[5] So, this is okay but now you have an image that doesn't even start with a CBFS header, which could also be a possible cause of failure. I think it would be good to test that while the unaligned files in this cannot be found, and aligned header somewhere in the middle of this would still be valid. (I think it'll be easiest to just memcpy() the CBFS entries together by hand for this, the create_cbfs() function can't cover all those special cases.)
https://review.coreboot.org/c/coreboot/+/56813/comment/ba05e5f7_1ca408e0 PS7, Line 472: assert_int_equal(0, size_out); This isn't true... in fact, I believe the current implementation will write the size claimed in the header to size_out, even if the mapping fails afterwards. But that should not be part of the API, the caller should just assume size_out is undefined if the mapping fails, so tests shouldn't check it in that case.
https://review.coreboot.org/c/coreboot/+/56813/comment/b5df61dc_1e5cc51e PS7, Line 475: static void test_cbfs_fail_beyond_rdev(void **state) This whole construction is so complicated that I think you should build a success case for it as well, just to make sure that the intentional failures you're seeing really fail for the reasons you think. Otherwise it's easily possible that there's something wrong in the setup that just makes everything you do with this test fail. (Would need to store expected success or failure in s->ex as well, then.)
https://review.coreboot.org/c/coreboot/+/56813/comment/9f79576d_09def42b PS7, Line 491: full_file_size I believe you want sizeof(test_file_to_go_beyond) here, because full_file_size may be much larger (and you'd end up copying invalid memory).
https://review.coreboot.org/c/coreboot/+/56813/comment/563882ce_90cd42da PS7, Line 492: test_file_1 Below you're looking for TEST_DATA_2_FILENAME, so I think you want &test_file_2?
https://review.coreboot.org/c/coreboot/+/56813/comment/5e0ba656_5752008a PS7, Line 526: mapping = cbfs_map("abcdefghijklmnop", &size_out); This is a good test to have, but it could still fail to check a problem where the code just strcmp()s over the end of the filename field and just doesn't match this file because there are more "letters" (misinterpreted bytes from the attributes or file data) that are not in the name you're looking for. A second, complementary test for that case could be done by copying test_file_1 into the image, then changing the (struct cbfs_file).offset field to be32(offsetof(filename) + strlen(TEST_DATA_1_FILENAME)) (which should come out one short of the null terminator and thus make it fail)).
https://review.coreboot.org/c/coreboot/+/56813/comment/84b3f4bd_6e78c559 PS7, Line 537: const struct cbfs_test_file test_file = { nit: maybe easier to copy in one of your pre-existing files and then just change one field?
https://review.coreboot.org/c/coreboot/+/56813/comment/0d3337a6_1e9ef159 PS7, Line 545: .offset = cpu_to_be32(sizeof(struct cbfs_file) + FILENAME_SIZE), Now that I think of it it would also be good to have another test where the attribute is cut off in the middle, i.e. after the first or second uint32_t. This could be achieved by making offset just 4 or 8 larger than attributes_offset.
https://review.coreboot.org/c/coreboot/+/56813/comment/adcf0ea3_7ce22a6f PS7, Line 617: .attributes_offset = cpu_to_be32( This should have no attributes so it doesn't overlap so much with the other test above.
https://review.coreboot.org/c/coreboot/+/56813/comment/2dd4167b_7ae7243d PS7, Line 730: static void test_cbfs_length_uint32max(void **state) Would be nice to have this for attributes_offset as well.
https://review.coreboot.org/c/coreboot/+/56813/comment/46a13460_361a93e7 PS7, Line 757: setup_test_cbfs_aligned I haven't checked exactly how this works, but can you use the group_setup/group_teardown stuff from cmocka_run_group_tests() to cover this? That would be a lot cleaner than having to duplicate all the boilerplate all over the place.
https://review.coreboot.org/c/coreboot/+/56813/comment/30b437bc_3394f0d3 PS7, Line 851: attrs_and_data Rather than only testing just the edge, it would be nice to also test a little in the middle (e.g. either 4 or 8 bytes further in, to see if having only the attribute tag or the tag and size throw it off somehow.).
https://review.coreboot.org/c/coreboot/+/56813/comment/e3f97ccc_884ff402 PS7, Line 852: CBFS_TYPE_NULL You say that but your code doesn't do it.
https://review.coreboot.org/c/coreboot/+/56813/comment/356b16dd_293ef5f8 PS7, Line 866: filename Same here, testing when the cutoff is a few bytes further in (but before the null terminator) would also be useful.