[Mesa-dev] [PATCH 1/5] gallium/u_inlines: fix member access within null pointer
Roland Scheidegger
sroland at vmware.com
Wed Feb 8 17:12:28 UTC 2017
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=>
>
>
More information about the mesa-dev
mailing list