Re: drm: fix blob pointer check

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

Hi Dan,

Thanks a lot for pointing this out. I saw you previous comment but 
didn't realize the issue.
I'll set the pointer to NULL.

I don't if there is an agreement on this, but do you think the 
unref/free functions should check for error pointers?

-
Lionel

On 11/03/16 11:39, Dan Carpenter wrote:
> Hi Lionel Landwerlin,
>
> The patch 562c5b4d8986: "drm: fix blob pointer check" from Mar 10,
> 2016, has a problem.
>
> drivers/gpu/drm/drm_atomic_helper.c
>    2924          blob = drm_property_create_blob(dev,
>    2925                                          sizeof(struct drm_color_lut) * size,
>    2926                                          NULL);
>    2927          if (IS_ERR(blob)) {
>    2928                  ret = PTR_ERR(blob);
>    2929                  goto fail;
>
> These types of goto fails are a trap for the unwary.
>
> I deliberately reported the bug instead of fixing it because I am a jerk
> and because last time when I did this jerky thing, people were not
> convinced that they would fall for the trap every single time.
>
> http://www.spinics.net/lists/cgroups/msg15262.html
>
>    2930          }
>    2931
>
> [ snip ]
>
>    2973  fail:
>    2974          if (ret == -EDEADLK)
>    2975                  goto backoff;
>    2976
>    2977          drm_atomic_state_free(state);
>    2978          drm_property_unreference_blob(blob);
>                                                ^^^^
> Blob is an error pointer here so it will oops inside the function call.
>
> The better way to write this is to unwind in the reverse order from the
> allocations.  So since we allocated state first then we free it last.
> Use explicit names based on what the goto does.  err_unreference:
> err_free_state:.  Don't free things that haven't been allocated.
>
> Or you could set "blob = NULL;" before the goto.
>
>    2979
>    2980          return;
>    2981  backoff:
>    2982          drm_atomic_state_clear(state);
>    2983          drm_atomic_legacy_backoff(state);
>    2984
>    2985          goto retry;
>    2986  }
>
> regards,
> dan carpenter
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel