Re: [PATCH spice-server 2/2] char device: Ensure public function parameters are not NULL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Thu, Nov 05, 2015 at 04:58:32PM +0100, Pavel Grunt wrote:
> ---
>  server/char_device.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  server/reds.c        |  2 ++
>  server/spicevmc.c    |  6 ++++++
>  3 files changed, 52 insertions(+), 3 deletions(-)
>
> diff --git a/server/char_device.c b/server/char_device.c
> index 54357f0..00ed4d9 100644
> --- a/server/char_device.c
> +++ b/server/char_device.c
> @@ -376,6 +376,9 @@ void spice_char_device_send_to_client_tokens_add(SpiceCharDeviceState *dev,
>  {
>      SpiceCharDeviceClientState *dev_client;
>
> +    spice_return_if_fail(dev != NULL);
> +    spice_return_if_fail(client != NULL);

The check for client is not necessary (I should not abort I think).
Logic bellow warn and return if spice_char_device_client_find returns
NULL (which it should for NULL client)

> +
>      dev_client = spice_char_device_client_find(dev, client);
>
>      if (!dev_client) {
> @@ -391,6 +394,9 @@ void spice_char_device_send_to_client_tokens_set(SpiceCharDeviceState *dev,
>  {
>      SpiceCharDeviceClientState *dev_client;
>
> +    spice_return_if_fail(dev != NULL);
> +    spice_return_if_fail(client != NULL);

Same.

> +
>      dev_client = spice_char_device_client_find(dev, client);
>
>      if (!dev_client) {
> @@ -573,6 +579,8 @@ SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_get(SpiceCharDeviceSt
>                                                                 RedClient *client,
>                                                                 int size)
>  {
> +    spice_return_val_if_fail(dev != NULL, NULL);
> +
>     return  __spice_char_device_write_buffer_get(dev, client, size,
>               client ? WRITE_BUFFER_ORIGIN_CLIENT : WRITE_BUFFER_ORIGIN_SERVER,
>               0);
> @@ -581,6 +589,8 @@ SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_get(SpiceCharDeviceSt
>  SpiceCharDeviceWriteBuffer *spice_char_device_write_buffer_get_server_no_token(
>      SpiceCharDeviceState *dev, int size)
>  {
> +    spice_return_val_if_fail(dev != NULL, NULL);
> +
>     return  __spice_char_device_write_buffer_get(dev, NULL, size,
>               WRITE_BUFFER_ORIGIN_SERVER_NO_TOKEN, 0);
>  }
> @@ -597,6 +607,7 @@ void spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
>                                          SpiceCharDeviceWriteBuffer *write_buf)
>  {
>      spice_assert(dev);
> +    spice_return_if_fail(write_buf != NULL);

I would not abort here as well.

>      /* caller shouldn't add buffers for client that was removed */
>      if (write_buf->origin == WRITE_BUFFER_ORIGIN_CLIENT &&
>          !spice_char_device_client_find(dev, write_buf->client)) {
> @@ -612,9 +623,15 @@ void spice_char_device_write_buffer_add(SpiceCharDeviceState *dev,
>  void spice_char_device_write_buffer_release(SpiceCharDeviceState *dev,
>                                              SpiceCharDeviceWriteBuffer *write_buf)
>  {
> -    int buf_origin = write_buf->origin;
> -    uint32_t buf_token_price = write_buf->token_price;
> -    RedClient *client = write_buf->client;
> +    int buf_origin;
> +    uint32_t buf_token_price;
> +    RedClient *client;
> +
> +    spice_return_if_fail(write_buf != NULL);

Not sure if abort here is okay

> +
> +    buf_origin = write_buf->origin;
> +    buf_token_price = write_buf->token_price;
> +    client = write_buf->client;
>
>      spice_assert(!ring_item_is_linked(&write_buf->link));
>      if (!dev) {
> @@ -690,6 +707,8 @@ SpiceCharDeviceState *spice_char_device_state_create(SpiceCharDeviceInstance *si
>  void spice_char_device_state_reset_dev_instance(SpiceCharDeviceState *state,
>                                                  SpiceCharDeviceInstance *sin)
>  {
> +    spice_return_if_fail(state != NULL);
> +    spice_return_if_fail(sin != NULL);
>      spice_debug("sin %p dev_state %p", sin, state);
>      state->sin = sin;
>      sin->st = state;
> @@ -697,6 +716,7 @@ void spice_char_device_state_reset_dev_instance(SpiceCharDeviceState *state,
>
>  void *spice_char_device_state_opaque_get(SpiceCharDeviceState *dev)
>  {
> +    spice_return_val_if_fail(dev != NULL, NULL);
>      return dev->opaque;
>  }
>
> @@ -721,6 +741,8 @@ static void spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
>
>  void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
>  {
> +    spice_return_if_fail(char_dev != NULL);
> +

I would just return in this case, maybe with a spice_warning to log it.

>      reds_on_char_device_state_destroy(char_dev);
>      if (char_dev->write_to_dev_timer) {
>          core->timer_remove(char_dev->write_to_dev_timer);
> @@ -796,6 +818,9 @@ void spice_char_device_client_remove(SpiceCharDeviceState *dev,
>  {
>      SpiceCharDeviceClientState *dev_client;
>  
> +    spice_return_if_fail(dev != NULL);
> +    spice_return_if_fail(client != NULL);

Is it necessary to abort when trying to remove a NULL client?

> +
>      spice_debug("dev_state %p client %p", dev, client);
>      dev_client = spice_char_device_client_find(dev, client);
>  
> @@ -814,11 +839,16 @@ void spice_char_device_client_remove(SpiceCharDeviceState *dev,
>  int spice_char_device_client_exists(SpiceCharDeviceState *dev,
>                                      RedClient *client)
>  {
> +    spice_return_val_if_fail(dev != NULL, FALSE);
> +    spice_return_val_if_fail(client != NULL, FALSE);

Is it necessary to abort when looking for a NULL client?
Before, spice_char_device_client_find would be NULL so function
would return false.

> +
>      return (spice_char_device_client_find(dev, client) != NULL);
>  }
>  
>  void spice_char_device_start(SpiceCharDeviceState *dev)
>  {
> +    spice_return_if_fail(dev != NULL);
> +
>      spice_debug("dev_state %p", dev);
>      dev->running = TRUE;
>      spice_char_device_state_ref(dev);
> @@ -829,6 +859,8 @@ void spice_char_device_start(SpiceCharDeviceState *dev)
>  
>  void spice_char_device_stop(SpiceCharDeviceState *dev)
>  {
> +    spice_return_if_fail(dev != NULL);
> +
>      spice_debug("dev_state %p", dev);
>      dev->running = FALSE;
>      dev->active = FALSE;
> @@ -841,6 +873,8 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
>  {
>      RingItem *client_item;
>  
> +    spice_return_if_fail(dev != NULL);
> +
>      spice_char_device_stop(dev);
>      dev->wait_for_migrate_data = FALSE;
>      spice_debug("dev_state %p", dev);
> @@ -871,6 +905,8 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
>  
>  void spice_char_device_wakeup(SpiceCharDeviceState *dev)
>  {
> +    spice_return_if_fail(dev != NULL);
> +
>      spice_char_device_write_to_device(dev);
>      spice_char_device_read_from_device(dev);
>  }
> @@ -883,6 +919,7 @@ void spice_char_device_state_migrate_data_marshall_empty(SpiceMarshaller *m)
>  {
>      SpiceMigrateDataCharDevice *mig_data;
>
> +    spice_return_if_fail(m != NULL);
>      spice_debug(NULL);
>      mig_data = (SpiceMigrateDataCharDevice *)spice_marshaller_reserve_space(m,
>                                                                              sizeof(*mig_data));
> @@ -907,6 +944,8 @@ void spice_char_device_state_migrate_data_marshall(SpiceCharDeviceState *dev,
>      uint32_t *write_to_dev_tokens_ptr;
>      SpiceMarshaller *m2;
>
> +    spice_return_if_fail(dev != NULL);
> +    spice_return_if_fail(m != NULL);
>      /* multi-clients are not supported */
>      spice_assert(dev->num_clients == 1);
>      client_state = SPICE_CONTAINEROF(ring_get_tail(&dev->clients),
> @@ -964,6 +1003,8 @@ int spice_char_device_state_restore(SpiceCharDeviceState *dev,
>      SpiceCharDeviceClientState *client_state;
>      uint32_t client_tokens_window;
>  
> +    spice_return_val_if_fail(dev != NULL, FALSE);
> +    spice_return_val_if_fail(mig_data != NULL, FALSE);
>      spice_assert(dev->num_clients == 1 && dev->wait_for_migrate_data);
>  
>      client_state = SPICE_CONTAINEROF(ring_get_tail(&dev->clients),
> diff --git a/server/reds.c b/server/reds.c
> index 1f6774e..4d18ca8 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2983,6 +2983,8 @@ static SpiceCharDeviceState *attach_to_red_agent(SpiceCharDeviceInstance *sin)
>  
>  SPICE_GNUC_VISIBLE void spice_server_char_device_wakeup(SpiceCharDeviceInstance* sin)
>  {
> +    spice_return_if_fail(sin != NULL);
> +
>      if (!sin->st) {
>          spice_warning("no SpiceCharDeviceState attached to instance %p", sin);
>          return;
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 6ac1561..0780192 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -508,6 +508,8 @@ SpiceCharDeviceState *spicevmc_device_connect(SpiceCharDeviceInstance *sin,
>      ClientCbs client_cbs = { NULL, };
>      SpiceCharDeviceCallbacks char_dev_cbs = {NULL, };
>
> +    spice_return_val_if_fail(sin != NULL, NULL);
> +
>      channel_cbs.config_socket = spicevmc_red_channel_client_config_socket;
>      channel_cbs.on_disconnect = spicevmc_red_channel_client_on_disconnect;
>      channel_cbs.send_item = spicevmc_red_channel_send_item;
> @@ -552,6 +554,8 @@ void spicevmc_device_disconnect(SpiceCharDeviceInstance *sin)
>  {
>      SpiceVmcState *state;
>
> +    spice_return_if_fail(sin != NULL);
> +
>      state = (SpiceVmcState *)spice_char_device_state_opaque_get(sin->st);
>
>      if (state->recv_from_client_buf) {
> @@ -569,6 +573,8 @@ SPICE_GNUC_VISIBLE void spice_server_port_event(SpiceCharDeviceInstance *sin, ui
>  {
>      SpiceVmcState *state;
>
> +    spice_return_if_fail(sin != NULL);
> +
>      if (sin->st == NULL) {
>          spice_warning("no SpiceCharDeviceState attached to instance %p", sin);
>          return;
> -- 
> 2.5.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Best,
  Victor Toso
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]