Re: [Mesa-dev] [PATCH 1/5] gallium/u_inlines: fix member access within null pointer

[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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev