[Mesa-dev] [PATCH 1/5] gallium/u_inlines: fix member access within null pointer
Roland Scheidegger
sroland at vmware.com
Wed Feb 8 19:14:30 UTC 2017
So, I'd be happy with either changing the code to check for null
pointers or using no_sanitize attribute. But it looks like others might
not agree...
Roland
Am 08.02.2017 um 19:21 schrieb Bartosz Tomczyk:
> 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
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__godbolt.org_g_JUzWyv&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=nJUAuTvZ9Uyp_5WTx9Fk7bdIyABDssOiC5qcPZTy07w&e=>
> 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
> <mailto: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 <mailto: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>
> >> <mailto: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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=zlOURfZFxmK2zVgrGtSzGuJuJTAuBy0QpU4hEYil8YA&e=>
> >>
> <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=
> <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>
> >> <mailto: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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=p-f7QQ_QHP5vyuF_HWZNYncb0xgyi-KX0R_PmH3z-Iw&e=>
> >>
> <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=
> <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>
> >> <mailto: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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=p-f7QQ_QHP5vyuF_HWZNYncb0xgyi-KX0R_PmH3z-Iw&e=>
> >>
> <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=
> <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>
> <mailto: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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=p-f7QQ_QHP5vyuF_HWZNYncb0xgyi-KX0R_PmH3z-Iw&e=>
> >>
> <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=
> <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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=p-f7QQ_QHP5vyuF_HWZNYncb0xgyi-KX0R_PmH3z-Iw&e=>
>
>
More information about the mesa-dev
mailing list