[Mesa-dev] [PATCH 1/5] gallium/u_inlines: fix member access within null pointer
Bartosz Tomczyk
bartosz.tomczyk86 at gmail.com
Wed Feb 8 15:54:24 UTC 2017
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>
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
> 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
> >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >>>
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> > _______________________________________________
> > 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/6062706f/attachment-0001.html>
More information about the mesa-dev
mailing list