[Mesa-dev] [PATCH 1/5] gallium/u_inlines: fix member access within null pointer
Marek Olšák
maraeo at gmail.com
Wed Feb 8 19:17:54 UTC 2017
The no_sanitize attribute seems to be the most acceptable approach.
Marek
On Wed, Feb 8, 2017 at 8:14 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> 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