re: drm: fix blob pointer check

re: drm: fix blob pointer check

[Dan Carpenter]

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

Re: drm: fix blob pointer check

[Lionel Landwerlin]

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

Re: drm: fix blob pointer check

[David Herrmann]

Hey

On Fri, Mar 11, 2016 at 12:52 PM, Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
> 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?

This is what we did so far, and I'd recommend keeping it for now:

        r = PTR_ERR(foobar);
        foobar = NULL;
        goto error;

As an option, stop overloading pointers and return them like everyone
else does: as a double-pointer in an output argument. This ERR_PTR()
craziness needs to stop.

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

Re: drm: fix blob pointer check

[Dan Carpenter]

On Fri, Mar 11, 2016 at 11:52:44AM +0000, Lionel Landwerlin wrote:
> 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?

I'm with the netdev people on this and think we shouldn't add sanity
checks to free functions.  For example, free_netdev().  If you always
keep track of what is allocated and what is not then the code is easier
to follow than if you start mixing them up and passing invalid pointers
to other functions.  If you handle errors right away that's simpler.

regards,
dan carpenter

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