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
>
