lkml.org 
[lkml]   [2014]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC PATCH] UBSan: run-time undefined behavior sanity checker
    On 10/22/2014 01:58 PM, Rasmus Villemoes wrote:
    > On Mon, Oct 20 2014, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
    >
    >> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
    >> Compiler inserts code that perform certain kinds of
    >> checks before operations that could cause UB.
    >> If check fails (i.e. UB detected) __ubsan_handle_* function called.
    >> to print error message.
    >>
    >> So the most of the work is done by compiler.
    >> This patch just implements ubsan handlers printing errors.
    >>
    >> GCC supports this since 4.9, however upcoming GCC 5.0 has
    >> more checkers implemented.
    >
    > [...]
    >
    >> +
    >> +#define REPORTED_BIT 31
    >> +#define COLUMN_MASK (~(1U << REPORTED_BIT))
    >> +
    >> +static bool is_disabled(struct source_location *location)
    >> +{
    >> + return test_and_set_bit(REPORTED_BIT,
    >> + (unsigned long *)&location->column);
    >> +}
    >
    > [...]
    >
    >> +struct source_location {
    >> + const char *file_name;
    >> + u32 line;
    >> + u32 column;
    >> +};
    >
    >
    > AFAICT, this introduces UB and/or memory corruption on big-endian
    > systems with BITS_PER_LONG==64. (Also, on both LE and BE 64 bit systems,
    > there's the issue of the alignment of location->column, which is likely
    > to be 4-but-not-8 byte aligned).
    >

    You are right. This should fix it:

    diff --git a/lib/ubsan.c b/lib/ubsan.c
    index 7788f47..cfdf017 100644
    --- a/lib/ubsan.c
    +++ b/lib/ubsan.c
    @@ -53,18 +53,24 @@ static bool handler_enabled(unsigned int handler)
    }

    #define REPORTED_BIT 31
    -#define COLUMN_MASK (~(1U << REPORTED_BIT))
    +
    +#if (BITS_PER_LONG == 64) && defined(__BIG_ENDIAN)
    +#define COLUMN_MASK (~(1U << REPORTED_BIT)
    +#define LINE_MASK (~0U)
    +#else
    +#define COLUMN_MASK (~0U)
    +#define LINE_MASK (~(1U << REPORTED_BIT))
    +#endif

    static bool is_disabled(struct source_location *location)
    {
    - return test_and_set_bit(REPORTED_BIT,
    - (unsigned long *)&location->column);
    + return test_and_set_bit(REPORTED_BIT, &location->reported);
    }

    static void print_source_location(const char *prefix, struct source_location *loc)
    {
    pr_err("%s %s:%d:%d\n", prefix, loc->file_name,
    - loc->line, loc->column & COLUMN_MASK);
    + loc->line & LINE_MASK, loc->column & COLUMN_MASK);
    }

    static bool type_is_int(struct type_descriptor *type)
    diff --git a/lib/ubsan.h b/lib/ubsan.h
    index e2d8634..8965591 100644
    --- a/lib/ubsan.h
    +++ b/lib/ubsan.h
    @@ -15,8 +15,13 @@ struct type_descriptor {

    struct source_location {
    const char *file_name;
    - u32 line;
    - u32 column;
    + union {
    + unsigned long reported;
    + struct {
    + u32 line;
    + u32 column;
    + };
    + };
    };

    struct overflow_data {

    > Is the layout of struct source_location dictated by gcc?
    >

    Yes.

    > Rasmus
    >



    \
     
     \ /
      Last update: 2014-10-22 13:41    [W:4.742 / U:0.424 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site