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