llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.46k stars 11.76k forks source link

-fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size() #60926

Open kees opened 1 year ago

kees commented 1 year ago

While -fsanitize-bounds is able to perform run-time bounds checking on fixed-size arrays (i.e. when __builtin_object_size(x, 1) does not return SIZE_MAX), it does not perform bounds checking when __builtin_dynamic_object_size(x, 1) is available.

For example, the attached program produces no bounds-checker warnings for the "dynamic size" case:

/* Build with -Wall -O2 -fstrict-flex-arrays=3 -fsanitize=bounds -fstrict-flex-arrays=3 */
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include <malloc.h>

#define noinline __attribute__((__noinline__))

volatile int zero = 0; /* used to stop optimizer from seeing constant expressions. */

#define report_size(p)      do {    \
    const size_t bdos = __builtin_dynamic_object_size(p, 1); \
    \
    if (__builtin_constant_p(bdos)) { \
        if (bdos == SIZE_MAX) { \
            printf("\n" #p " has unknowable size\n"); \
        } else { \
            printf("\n" #p " has a fixed size: %zu (%zu elements of size %zu)\n", bdos, \
                   bdos / sizeof(*(p)), sizeof(*(p))); \
        } \
    } else { \
        printf("\n" #p " has a dynamic size: %zu (%zu elements of size %zu)\n", bdos, \
               bdos / sizeof(*(p)), sizeof(*(p))); \
    } \
} while (0)

#define report_assignment(p, index, expect)     \
    printf(#p "[%d] assignment: %d (should be %s)\n", index, (p)[index] = 0xFF, expect)

#define MAX_INDEX       16

struct fixed {
    unsigned long flags;
    size_t foo;
    int array[MAX_INDEX];
};

/* should emit "fixed" */
static void noinline do_fixed(struct fixed *p, int index)
{
    report_size(p->array);
    report_assignment(p->array, 0,      "ok");
    report_assignment(p->array, index,  "failure");
}

struct flex {
    unsigned long flags;
    size_t foo;
    int array[];
};

/* should emit "dynamic" */
static void noinline do_dynamic(unsigned char count, int index)
{
    /* malloc() is marked with __attribute__((alloc_size(1))) */
    struct flex *p = malloc(sizeof(*p) + (zero + count) * sizeof(*p->array));
    report_size(p->array);
    report_assignment(p->array, 0,      "ok");
    report_assignment(p->array, index,  "failure");
    free(p);
}

/* should emit "unknowable" */
static void noinline do_unknown(struct flex *p, int index)
{
    report_size(p->array);
    report_assignment(p->array, 0,      "ignored");
    report_assignment(p->array, index,  "ignored");
}

int main(int argc, char *argv[])
{
        int a;
        struct fixed f;
        int b;
        struct flex *p;
        int index = MAX_INDEX + zero;

        a = b = argc; /* wrap "f" with an extra int to catch overflow writes */
        do_fixed(&f, index + a - b);

        do_dynamic(index, index);

        p = malloc(100);
        do_unknown(p, index);
        free(p);

        return 0;
}
$ gcc -Wall -O2 -fstrict-flex-arrays=3 -fsanitize=bounds -fstrict-flex-arrays=3 -o bounds bounds.c
$ ./bounds

p->array has a fixed size: 64 (16 elements of size 4)
p->array[0] assignment: 255 (should be ok)
bounds.c:43:5: runtime error: index 16 out of bounds for type 'int[16]'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior bounds.c:43:5 in 
p->array[16] assignment: 255 (should be failure)

p->array has a dynamic size: 64 (16 elements of size 4)
p->array[0] assignment: 255 (should be ok)
p->array[16] assignment: 255 (should be failure)

p->array has unknowable size
p->array[0] assignment: 255 (should be ignored)
p->array[16] assignment: 255 (should be ignored)
kees commented 1 year ago

The matching GCC bug is: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

kees commented 1 year ago

This appears to actually be covered by -fsanitize=object-size, though it doesn't appear to have a warning mode. It is trap-only.

bwendling commented 1 year ago

From what I can grok, you want the sanitizer to generate a check for a dynamically allocated array access when the alloc_size attribute is used (via malloc)? I don't think such a check is dependent upon the __builtin_{dynamic_}object_size builtins. A fixed sized array is "easy" because the size is part of its type.

It might be "hackable" to add the alloc_size to the array so that it's available. (Though as I mentioned privately, that information might be lost if the array is passed outside of a function...maybe...) This is similar to the element_count hack I wrote (https://reviews.llvm.org/D148381).

Would the above be sufficient for your needs?

I wasn't able to replicate a trap with -fsanitize=object-size.