linux-kernel - Re: [PATCH] input: ims-pcu: remove redundant assignment to variable 'error'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Jun 2020 17:45:07 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Colin Ian King <colin.king@...onical.com>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-input@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] input: ims-pcu: remove redundant assignment to variable
 'error'

On Wed, Jun 03, 2020 at 03:18:46PM +0100, Colin Ian King wrote:
> On 03/06/2020 15:09, Dan Carpenter wrote:
> > On Wed, Jun 03, 2020 at 02:51:02PM +0100, Colin King wrote:
> >> From: Colin Ian King <colin.king@...onical.com>
> >>
> >> The variable error is being initialized with a value that is
> >> never read and the -ENOMEM error return is being returned anyhow
> >> by the error exit path to label err_free_mem.  The assignment is
> >> redundant and can be removed.
> >>
> >> Addresses-Coverity: ("Unused value")
> >> Signed-off-by: Colin Ian King <colin.king@...onical.com>
> >> ---
> >>  drivers/input/misc/ims-pcu.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> >> index d8dbfc030d0f..4ba68aa3d281 100644
> >> --- a/drivers/input/misc/ims-pcu.c
> >> +++ b/drivers/input/misc/ims-pcu.c
> >> @@ -292,7 +292,6 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
> >>  	if (!gamepad || !input) {
> >>  		dev_err(pcu->dev,
> >>  			"Not enough memory for gamepad device\n");
> >> -		error = -ENOMEM;
> >>  		goto err_free_mem;
> > 
> > It would be better to change the return instead.
> > 
> > regards,
> > dan carpenter
> > 
> 
> I'm not sure about that, the err_free_mem path is used by another error
> exit return path that also needs to free the device and gamepad and
> returns ENOMEM, so I think this is a good enough shared error exit strategy.

The code looks like this:

drivers/input/misc/ims-pcu.c
   284  static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
   285  {
   286          struct ims_pcu_gamepad *gamepad;
   287          struct input_dev *input;
   288          int error;
   289  
   290          gamepad = kzalloc(sizeof(struct ims_pcu_gamepad), GFP_KERNEL);
   291          input = input_allocate_device();
   292          if (!gamepad || !input) {
   293                  dev_err(pcu->dev,
   294                          "Not enough memory for gamepad device\n");
   295                  error = -ENOMEM;
   296                  goto err_free_mem;

The "error" is always set before all the gotos.

   297          }
   298  
   299          gamepad->input = input;
   300  
   301          snprintf(gamepad->name, sizeof(gamepad->name),
   302                   "IMS PCU#%d Gamepad Interface", pcu->device_no);
   303  
   304          usb_make_path(pcu->udev, gamepad->phys, sizeof(gamepad->phys));
   305          strlcat(gamepad->phys, "/input1", sizeof(gamepad->phys));
   306  
   307          input->name = gamepad->name;
   308          input->phys = gamepad->phys;
   309          usb_to_input_id(pcu->udev, &input->id);
   310          input->dev.parent = &pcu->ctrl_intf->dev;
   311  
   312          __set_bit(EV_KEY, input->evbit);
   313          __set_bit(BTN_A, input->keybit);
   314          __set_bit(BTN_B, input->keybit);
   315          __set_bit(BTN_X, input->keybit);
   316          __set_bit(BTN_Y, input->keybit);
   317          __set_bit(BTN_START, input->keybit);
   318          __set_bit(BTN_SELECT, input->keybit);
   319  
   320          __set_bit(EV_ABS, input->evbit);
   321          input_set_abs_params(input, ABS_X, -1, 1, 0, 0);
   322          input_set_abs_params(input, ABS_Y, -1, 1, 0, 0);
   323  
   324          error = input_register_device(input);
   325          if (error) {
   326                  dev_err(pcu->dev,
   327                          "Failed to register gamepad input device: %d\n",
   328                          error);
   329                  goto err_free_mem;

The input_register_device() can return a bunch of different error codes.
Better to preserve them.  "error" is set.

   330          }
   331  
   332          pcu->gamepad = gamepad;
   333          return 0;
   334  
   335  err_free_mem:
   336          input_free_device(input);
   337          kfree(gamepad);
   338          return -ENOMEM;

We just change this from "return -ENOMEM;" to "return error;"

   339  }

regards,
dan carpenter