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