[Mesa-dev] [PATCH 1/5] gallium/u_inlines: fix member access within null pointer
Bartosz Tomczyk
bartosz.tomczyk86 at gmail.com
Wed Feb 8 18:21:59 UTC 2017
I find ASAN and UBSAN extremely useful while debugging, but currently there
is hundreds issues reported while running simple test. That make it very
hard to use it.
As Brian mention, first argument is never NULL. It can simplify changes
while still make UBSAN happy.
Roland, yes compiler will optimize it anyway, and will generate same code.
Please take a look at https://godbolt.org/g/JUzWyv
just uncomment //#define USE_SAFE and observe final asm output.
On Wed, Feb 8, 2017 at 6:21 PM, Marek Olšák <maraeo at gmail.com> wrote:
> FWIW, I'd like us to focus on real issues instead of trying to please
> static analyzers.
>
> If one of the reference functions stops working, we'll know that
> pretty quickly. Until then, there is nothing to do.
>
> Marek
>
> On Wed, Feb 8, 2017 at 6:12 PM, Roland Scheidegger <sroland at vmware.com>
> wrote:
> > no_sanitize attribute looks like a reasonable solution to me (as long as
> > it still compiles everywhere, of course).
> > (But as said, since this is iffy, I'd be ok with changing the code too,
> > iff you can prove that compilers optimize this away.)
> >
> > Roland
> >
> > Am 08.02.2017 um 16:54 schrieb Bartosz Tomczyk:
> >> I'm not insist to push it, although it looks like undefined behavior by
> >> C standard, all known compilers generates perfectly legal code for those
> >> construction. If there is strong objections about the patches, how
> >> about disabling UBSAN for those function
> >> with __attribute__((no_sanitize("undefined"))) ?
> >>
> >> On Wed, Feb 8, 2017 at 3:48 PM, Roland Scheidegger <sroland at vmware.com
> >> <mailto:sroland at vmware.com>> wrote:
> >>
> >> Am 08.02.2017 um 10:21 schrieb Nicolai Hähnle:
> >> > On 07.02.2017 23:00, Brian Paul wrote:
> >> >> Yeah, it would never make sense to pass NULL as the first
> argument to
> >> >> any of the _reference() functions.
> >> >>
> >> >> Would putting an assert(ptr) at the start of
> pipe_surface_reference(),
> >> >> for example, silence the UBSAN warning?
> >> >
> >> > I think the point is that *ptr is NULL, and so (*ptr)->reference
> >> > "accesses a member within a NULL pointer".
> >> >
> >> > Yes, it's only for taking the address, but perhaps UBSAN doesn't
> care
> >> > about that?
> >> >
> >> > I'm not enough of a C language lawyer to know whether that's
> genuinely
> >> > undefined behavior or if this is an UBSAN false positive.
> >> Actually that seems to be a complicated question. This article here
> has
> >> a nice summary why it is undefined behavior:
> >> https://software.intel.com/en-us/blogs/2015/04/20/null-
> pointer-dereferencing-causes-undefined-behavior
> >> <https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__software.intel.com_en-2Dus_blogs_2015_04_20_null-
> 2Dpointer-2Ddereferencing-2Dcauses-2Dundefined-2Dbehavior&d=DwMFaQ&c=
> uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=
> OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=ZWiQfyS52fxj0JPR5C-
> cJ5wN4H2ko91jFdFqgEdWlfE&e=>
> >> Plus the comments why the article is wrong and it's perfectly
> defined...
> >>
> >> >
> >> > The patch as-is is overzealous (the pptr ? *pptr : NULL part is
> >> > unnecessary), but the part of replacing &(*ptr)->reference by
> *ptr ?
> >> > &(*ptr)->reference : NULL and similarly for surf may be
> warranted, and
> >> > is likely to be optimized away (though somebody should verify
> that...).
> >> So, if the compiler does optimize it away (with ordinary
> optimization
> >> switched on), I'm open to changes there. Otherwise I still don't
> think
> >> it's a good idea - basically the idea is that reference counting is
> used
> >> everywhere, so it should be as cheap as possible. (Note that this is
> >> used in a lot of places with an explicit NULL pointer as the second
> >> argument, and noone has ever claimed it caused actual issues.)
> >>
> >> Roland
> >>
> >>
> >> >
> >> > Cheers,
> >> > Nicolai
> >> >
> >> >> -Brian
> >> >>
> >> >>
> >> >> On 02/07/2017 02:45 PM, Roland Scheidegger wrote:
> >> >>> I'm not quite sure there's really a bug here?
> >> >>> As far as I can tell, these functions are architected
> >> specifically to
> >> >>> look like that - they do not actually dereference potential null
> >> >>> pointers, but only take the address in the end. This change
> >> seems to add
> >> >>> some overhead.
> >> >>>
> >> >>>
> >> >>> Roland
> >> >>>
> >> >>>
> >> >>> Am 07.02.2017 um 19:34 schrieb Bartosz Tomczyk:
> >> >>>> ---
> >> >>>> src/gallium/auxiliary/util/u_inlines.h | 65
> >> >>>> ++++++++++++++++++++--------------
> >> >>>> 1 file changed, 39 insertions(+), 26 deletions(-)
> >> >>>>
> >> >>>> diff --git a/src/gallium/auxiliary/util/u_inlines.h
> >> >>>> b/src/gallium/auxiliary/util/u_inlines.h
> >> >>>> index b7b8313583..3bb3bcd6e0 100644
> >> >>>> --- a/src/gallium/auxiliary/util/u_inlines.h
> >> >>>> +++ b/src/gallium/auxiliary/util/u_inlines.h
> >> >>>> @@ -104,14 +104,17 @@ pipe_reference(struct pipe_reference
> *ptr,
> >> >>>> struct pipe_reference *reference)
> >> >>>> }
> >> >>>>
> >> >>>> static inline void
> >> >>>> -pipe_surface_reference(struct pipe_surface **ptr, struct
> >> >>>> pipe_surface *surf)
> >> >>>> +pipe_surface_reference(struct pipe_surface **pptr, struct
> >> >>>> pipe_surface *surf)
> >> >>>> {
> >> >>>> - struct pipe_surface *old_surf = *ptr;
> >> >>>> + struct pipe_surface *ptr = pptr ? *pptr : NULL;
> >> >>>> + struct pipe_surface *old_surf = ptr;
> >> >>>>
> >> >>>> - if (pipe_reference_described(&(*ptr)->reference,
> >> &surf->reference,
> >> >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL,
> >> >>>> + surf ? &surf->reference :
> NULL,
> >> >>>>
> >> >>>> (debug_reference_descriptor)debug_describe_surface))
> >> >>>> old_surf->context->surface_destroy(old_surf->context,
> >> >>>> old_surf);
> >> >>>> - *ptr = surf;
> >> >>>> +
> >> >>>> + if (pptr) *pptr = surf;
> >> >>>> }
> >> >>>>
> >> >>>> /**
> >> >>>> @@ -121,37 +124,43 @@ pipe_surface_reference(struct
> pipe_surface
> >> >>>> **ptr, struct pipe_surface *surf)
> >> >>>> * that's shared by multiple contexts.
> >> >>>> */
> >> >>>> static inline void
> >> >>>> -pipe_surface_release(struct pipe_context *pipe, struct
> >> pipe_surface
> >> >>>> **ptr)
> >> >>>> +pipe_surface_release(struct pipe_context *pipe, struct
> >> pipe_surface
> >> >>>> **pptr)
> >> >>>> {
> >> >>>> - if (pipe_reference_described(&(*ptr)->reference, NULL,
> >> >>>> + struct pipe_surface *ptr = pptr ? *pptr : NULL;
> >> >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL,
> >> NULL,
> >> >>>>
> >> >>>> (debug_reference_descriptor)debug_describe_surface))
> >> >>>> - pipe->surface_destroy(pipe, *ptr);
> >> >>>> - *ptr = NULL;
> >> >>>> + pipe->surface_destroy(pipe, ptr);
> >> >>>> + if (pptr) *pptr = NULL;
> >> >>>> }
> >> >>>>
> >> >>>>
> >> >>>> static inline void
> >> >>>> -pipe_resource_reference(struct pipe_resource **ptr, struct
> >> >>>> pipe_resource *tex)
> >> >>>> +pipe_resource_reference(struct pipe_resource **pptr, struct
> >> >>>> pipe_resource *tex)
> >> >>>> {
> >> >>>> - struct pipe_resource *old_tex = *ptr;
> >> >>>> + struct pipe_resource *ptr = pptr ? *pptr : NULL;
> >> >>>> + struct pipe_resource *old_tex = ptr;
> >> >>>>
> >> >>>> - if (pipe_reference_described(&(*ptr)->reference,
> >> &tex->reference,
> >> >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL,
> >> >>>> + tex ? &tex->reference : NULL,
> >> >>>>
> >> >>>> (debug_reference_descriptor)debug_describe_resource)) {
> >> >>>> pipe_resource_reference(&old_tex->next, NULL);
> >> >>>> old_tex->screen->resource_destroy(old_tex->screen,
> >> old_tex);
> >> >>>> }
> >> >>>> - *ptr = tex;
> >> >>>> +
> >> >>>> + if (pptr) *pptr = tex;
> >> >>>> }
> >> >>>>
> >> >>>> static inline void
> >> >>>> -pipe_sampler_view_reference(struct pipe_sampler_view **ptr,
> struct
> >> >>>> pipe_sampler_view *view)
> >> >>>> +pipe_sampler_view_reference(struct pipe_sampler_view **pptr,
> >> struct
> >> >>>> pipe_sampler_view *view)
> >> >>>> {
> >> >>>> - struct pipe_sampler_view *old_view = *ptr;
> >> >>>> + struct pipe_sampler_view *ptr = pptr ? *pptr : NULL;
> >> >>>> + struct pipe_sampler_view *old_view = ptr;
> >> >>>>
> >> >>>> - if (pipe_reference_described(&(*ptr)->reference,
> >> &view->reference,
> >> >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL,
> >> >>>> + view ? &view->reference :
> NULL,
> >> >>>>
> >> >>>> (debug_reference_descriptor)debug_describe_sampler_view))
> >> >>>> old_view->context->sampler_view_destroy(old_view->
> context,
> >> >>>> old_view);
> >> >>>> - *ptr = view;
> >> >>>> + if (pptr) *pptr = view;
> >> >>>> }
> >> >>>>
> >> >>>> /**
> >> >>>> @@ -162,29 +171,33 @@ pipe_sampler_view_reference(struct
> >> >>>> pipe_sampler_view **ptr, struct pipe_sampler_
> >> >>>> */
> >> >>>> static inline void
> >> >>>> pipe_sampler_view_release(struct pipe_context *ctx,
> >> >>>> - struct pipe_sampler_view **ptr)
> >> >>>> + struct pipe_sampler_view **pptr)
> >> >>>> {
> >> >>>> - struct pipe_sampler_view *old_view = *ptr;
> >> >>>> - if (*ptr && (*ptr)->context != ctx) {
> >> >>>> + struct pipe_sampler_view *ptr = pptr ? *pptr : NULL;
> >> >>>> + struct pipe_sampler_view *old_view = ptr;
> >> >>>> +
> >> >>>> + if (ptr && ptr->context != ctx) {
> >> >>>> debug_printf_once(("context mis-match in
> >> >>>> pipe_sampler_view_release()\n"));
> >> >>>> }
> >> >>>> - if (pipe_reference_described(&(*ptr)->reference, NULL,
> >> >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL,
> >> NULL,
> >> >>>>
> >> >>>> (debug_reference_descriptor)debug_describe_sampler_view)) {
> >> >>>> ctx->sampler_view_destroy(ctx, old_view);
> >> >>>> }
> >> >>>> - *ptr = NULL;
> >> >>>> + if(pptr) *pptr = NULL;
> >> >>>> }
> >> >>>>
> >> >>>> static inline void
> >> >>>> -pipe_so_target_reference(struct pipe_stream_output_target
> **ptr,
> >> >>>> +pipe_so_target_reference(struct pipe_stream_output_target
> **pptr,
> >> >>>> struct pipe_stream_output_target
> >> *target)
> >> >>>> {
> >> >>>> - struct pipe_stream_output_target *old = *ptr;
> >> >>>> + struct pipe_stream_output_target *ptr = pptr ? *pptr :
> NULL;
> >> >>>> + struct pipe_stream_output_target *old = ptr;
> >> >>>>
> >> >>>> - if (pipe_reference_described(&(*ptr)->reference,
> >> >>>> &target->reference,
> >> >>>> -
> >> >>>> (debug_reference_descriptor)debug_describe_so_target))
> >> >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL,
> >> >>>> + target ? &target->reference :
> >> NULL,
> >> >>>> +
> >> >>>> (debug_reference_descriptor)debug_describe_so_target))
> >> >>>>
> >> old->context->stream_output_target_destroy(old->context, old);
> >> >>>> - *ptr = target;
> >> >>>> + if (pptr) *pptr = target;
> >> >>>> }
> >> >>>>
> >> >>>> static inline void
> >> >>>>
> >> >>>
> >> >>> _______________________________________________
> >> >>> mesa-dev mailing list
> >> >>> mesa-dev at lists.freedesktop.org
> >> <mailto:mesa-dev at lists.freedesktop.org>
> >> >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >> <https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=
> uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=
> OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-
> iE1tTVWXaWKCNeTTo&e=>
> >> >>>
> >> >>
> >> >> _______________________________________________
> >> >> mesa-dev mailing list
> >> >> mesa-dev at lists.freedesktop.org
> >> <mailto:mesa-dev at lists.freedesktop.org>
> >> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >> <https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=
> uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=
> OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-
> iE1tTVWXaWKCNeTTo&e=>
> >> >
> >> > _______________________________________________
> >> > mesa-dev mailing list
> >> > mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.
> freedesktop.org>
> >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >> <https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=
> uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=
> OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-
> iE1tTVWXaWKCNeTTo&e=>
> >>
> >>
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170208/ceb4ec37/attachment-0001.html>
More information about the mesa-dev
mailing list