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.
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. 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 or p->foo was not set to any value as in test 3 and 4 ==== 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? thanks. Qing > On May 26, 2023, at 4:40 PM, Kees Cook <keesc...@chromium.org> wrote: > > On Thu, May 25, 2023 at 04:14:47PM +0000, Qing Zhao wrote: >> GCC will pass the number of elements info from the attached attribute to >> both >> __builtin_dynamic_object_size and bounds sanitizer to check the out-of-bounds >> or dynamic object size issues during runtime for flexible array members. >> >> This new feature will provide nice protection to flexible array members >> (which >> currently are completely ignored by both __builtin_dynamic_object_size and >> bounds sanitizers). > > Testing went pretty well, though I think I found some bdos issues: > > - some things that bdos can't know the size of, and correctly returned > SIZE_MAX in the past, now thinks are 0-sized. > - while bdos correctly knows the size of an element_count-annotated > flexible array, it doesn't know the size of the containing object > (i.e. it returns SIZE_MAX). > > Also, I think I found a precedence issue: > > - if both __alloc_size and 'element_count' are in use, the _smallest_ > of the two is what I would expect to be enforced by the sanitizer > and reported by __bdos. As is, alloc_size appears to be used when > it is available, regardless of what 'element_count' shows. > > I've updated my test cases to show it more clearly, but here is the > before/after: > > > GCC 13 (correctly does not implement "element_count"): > > $ ./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 > ok 3 global.unknown_size_unknown_to_bdos > 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 > not 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 > > > ToT GCC + this element_count series: > > $ ./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 > > > Test suite is here: > https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c > > -- > Kees Cook
