On Thu, Jul 06, 2023 at 06:56:21PM +0000, Qing Zhao wrote: > Hi, Kees, > > I have updated my V1 patch with the following changes: > A. changed the name to "counted_by" > B. changed the argument from a string to an identifier > C. updated the documentation and testing cases accordingly.
Sounds great! > > And then used this new gcc to test > https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c (with > the following change) > [opc@qinzhao-ol8u3-x86 Kees]$ !1091 > diff array-bounds.c array-bounds.c.org > 32c32 > < # define __counted_by(member) __attribute__((counted_by (member))) > --- > > # define __counted_by(member) > > __attribute__((__element_count__(#member))) > 34c34 > < # define __counted_by(member) __attribute__((counted_by (member))) > --- > > # define __counted_by(member) /* > > __attribute__((__element_count__(#member))) */ > > Then I got the following result: > [opc@qinzhao-ol8u3-x86 Kees]$ ./array-bounds 2>&1 | grep -v ^'#' > TAP version 13 > 1..12 > ok 1 global.fixed_size_seen_by_bdos > ok 2 global.fixed_size_enforced_by_sanitizer > not ok 3 global.unknown_size_unknown_to_bdos > not ok 4 global.unknown_size_ignored_by_sanitizer > ok 5 global.alloc_size_seen_by_bdos > ok 6 global.alloc_size_enforced_by_sanitizer > not ok 7 global.element_count_seen_by_bdos > ok 8 global.element_count_enforced_by_sanitizer > not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos > not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer > ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos > ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer > > The same as your previous results. Then I took a look at all the failed > testing: 3, 4, 7, 9, and 10. And studied the reasons for all of them. > > in a summary, there are two major issues: > 1. The reason for the failed testing 7 is the same issue as I observed in > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557 > Which is not a bug, it’s an expected behavior. In the bug, the problem is that "p" isn't known to be allocated, if I'm reading that correctly? I'm not sure this is a reasonable behavior, but let me get into the specific test, which looks like this: TEST(counted_by_seen_by_bdos) { struct annotated *p; int index = MAX_INDEX + unconst; p = alloc_annotated(index); REPORT_SIZE(p->array); /* 1 */ EXPECT_EQ(sizeof(*p), offsetof(typeof(*p), array)); /* Check array size alone. */ /* 2 */ EXPECT_EQ(__builtin_object_size(p->array, 1), SIZE_MAX); /* 3 */ EXPECT_EQ(__builtin_dynamic_object_size(p->array, 1), p->foo * sizeof(*p->array)); /* Check check entire object size. */ /* 4 */ EXPECT_EQ(__builtin_object_size(p, 1), SIZE_MAX); /* 5 */ EXPECT_EQ(__builtin_dynamic_object_size(p, 1), sizeof(*p) + p->foo * sizeof(*p->array)); } Test 1 trivially passes -- this is just a sanity check. Test 2 should be SIZE_MAX because __bos only knows compile-time sizes. Test 3 should pass: counted_by knows the allocation size and can be queried as part of the "p" object. Test 4 should be SIZE_MAX because __bos only knows compile-time sizes. Test 5 should pass as well, since, again, p can be examined. Passing p to __bdos implies it is allocated and the __counted_by annotation can be examined. If "return p->array[index];" would be caught by the sanitizer, then it follows that __builtin_dynamic_object_size(p, 1) must also know the size. i.e. both must examine "p" and its trailing flexible array and __counted_by annotation. > > 2. The common issue for the failed testing 3, 4, 9, 10 is: > > for the following annotated structure: > > ==== > struct annotated { > unsigned long flags; > size_t foo; > int array[] __attribute__((counted_by (foo))); > }; > > > struct annotated *p; > int index = 16; > > p = malloc(sizeof(*p) + index * sizeof(*p->array)); // allocated real size > > p->foo = index + 2; // p->foo was set by a different value than the real > size of p->array as in test 9 and 10 Right, tests 9 and 10 are checking that the _smallest_ possible value of the array is used. (There are two sources of information: the allocation size and the size calculated by counted_by. The smaller of the two should be used when both are available.) > or > p->foo was not set to any value as in test 3 and 4 For tests 3 and 4, yes, this was my mistake. I have fixed up the tests now. Bill noticed the same issue. Sorry for the think-o! > ==== > > i.e, the value of p->foo is NOT synced with the number of elements allocated > for the array p->array. > > I think that this should be considered as an user error, and the > documentation of the attribute should include > this requirement. (In the LLVM’s RFC, such requirement was included in the > programing model: > https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18) > > We can add a new warning option -Wcounted-by to report such user error if > needed. > > What’s your opinion on this? I think it is correct to allow mismatch between allocation and counted_by as long as only the least of the two is used. This may be desirable in a few situations. One example would be a large allocation that is slowly filled up by the program. I.e. the counted_by member is slowly increased during runtime (but not beyond the true allocation size). Of course allocation size is only available in limited situations, so the loss of that info is fine: we have counted_by for everything else. -- Kees Cook
