[bug report] rpmsg: Handle invalid parameters in public API

[bug report] rpmsg: Handle invalid parameters in public API

[Dan Carpenter]

Hello Bjorn Andersson,

The patch 93e9324431c9: "rpmsg: Handle invalid parameters in public
API" from Oct 18, 2016, leads to the following static checker warning:

	drivers/rpmsg/rpmsg_core.c:421 rpmsg_dev_probe()
	error: 'ept' dereferencing possible ERR_PTR()

drivers/rpmsg/rpmsg_core.c
    60   * Drivers should provide their @rpdev channel (so the new endpoint would belong
    61   * to the same remote processor their channel belongs to), an rx callback
    62   * function, an optional private data (which is provided back when the
    63   * rx callback is invoked), and an address they want to bind with the
    64   * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
    65   * dynamically assign them an available rpmsg address (drivers should have
    66   * a very good reason why not to always use RPMSG_ADDR_ANY here).
    67   *
    68   * Returns a pointer to the endpoint on success, or NULL on error.
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    69   */
    70  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
    71                                          rpmsg_rx_cb_t cb, void *priv,
    72                                          struct rpmsg_channel_info chinfo)
    73  {
    74          if (WARN_ON(!rpdev))
    75                  return ERR_PTR(-EINVAL);

The callers aren't expecting error pointers.  I could filter out this
one because Smatch knows it's impossible for that caller because rpdev
isn't NULL, but I feel like this should really return NULL.

    76  
    77          return rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
    78  }

regards,
dan carpenter

Re: [bug report] rpmsg: Handle invalid parameters in public API

[Bjorn Andersson]

On Sat 11 Feb 00:40 PST 2017, Dan Carpenter wrote:

> Hello Bjorn Andersson,
> 
> The patch 93e9324431c9: "rpmsg: Handle invalid parameters in public
> API" from Oct 18, 2016, leads to the following static checker warning:
> 
> 	drivers/rpmsg/rpmsg_core.c:421 rpmsg_dev_probe()
> 	error: 'ept' dereferencing possible ERR_PTR()
> 
> drivers/rpmsg/rpmsg_core.c
>     60   * Drivers should provide their @rpdev channel (so the new endpoint would belong
>     61   * to the same remote processor their channel belongs to), an rx callback
>     62   * function, an optional private data (which is provided back when the
>     63   * rx callback is invoked), and an address they want to bind with the
>     64   * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
>     65   * dynamically assign them an available rpmsg address (drivers should have
>     66   * a very good reason why not to always use RPMSG_ADDR_ANY here).
>     67   *
>     68   * Returns a pointer to the endpoint on success, or NULL on error.
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>     69   */
>     70  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
>     71                                          rpmsg_rx_cb_t cb, void *priv,
>     72                                          struct rpmsg_channel_info chinfo)
>     73  {
>     74          if (WARN_ON(!rpdev))
>     75                  return ERR_PTR(-EINVAL);
> 
> The callers aren't expecting error pointers.  I could filter out this
> one because Smatch knows it's impossible for that caller because rpdev
> isn't NULL, but I feel like this should really return NULL.
> 

I think it would make sense to report an ERR_PTR() from
rpmsg_create_ept(), but that's not what's currently expected.

I applied a patch, correcting the return value, to linux-next.

Thanks,
Bjorn

Re: [bug report] rpmsg: Handle invalid parameters in public API

[Dan Carpenter]

On Mon, Feb 13, 2017 at 08:58:09AM -0800, Bjorn Andersson wrote:
> >     70  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
> >     71                                          rpmsg_rx_cb_t cb, void *priv,
> >     72                                          struct rpmsg_channel_info chinfo)
> >     73  {
> >     74          if (WARN_ON(!rpdev))
> >     75                  return ERR_PTR(-EINVAL);
> > 
> > The callers aren't expecting error pointers.  I could filter out this
> > one because Smatch knows it's impossible for that caller because rpdev
> > isn't NULL, but I feel like this should really return NULL.
> > 
> 
> I think it would make sense to report an ERR_PTR() from
> rpmsg_create_ept(), but that's not what's currently expected.
> 

It sounds like you're trying to argue or persuade me but that's like
arguing with a fruit fly.  Complete waste of time.  I have invested
zero thought into this and had literally already forgotten sending this
email.  Just do whatever you think is best.  ;)

regards,
dan carpenter