[Mesa-dev] [PATCH 1/5] gallium/u_inlines: fix member access within null pointer
Nicolai Hähnle
nhaehnle at gmail.com
Wed Feb 8 09:21:15 UTC 2017
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.
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...).
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
More information about the mesa-dev
mailing list