On Thu, 29 May 2008, Dmitry Torokhov wrote:
> Hi Julia,
>
> On Thu, May 29, 2008 at 03:05:07PM +0200, Julia Lawall wrote:
> > From: Julia Lawall <julia@xxxxxxx>
> >
> > The variable report is only non-NULL and non-freed in a small region of
> > code, so it should only be freed in error handling code that comes from
> > that region.
> >
>
> Thank you for your patch. There isn't a double-kfree though, as far
> as I can see.
There was a double free because the error handling at the end of the
function (for input_register_device) would fall through to the
kfree(report) under err_free_devs, even though report is already freed at
that point.
Your solution does indeed look more elegant, and seems to preserve the
semantics of the original code.
julia
> Because we need to free the report in both success and
> failure cases the error handling is a bit unwieldy I agree. I don't
> want to apply your patch though because I don't like when we bypass
> parts of error handling path that weren't bypassed if we aborted
> earlier, if you follow me.
>
> What do you think about the patch below?
>
> --
> Dmitry
>
> Input: gtco - clean up error handling in gtco_probe
>
> Thanks to Julia Lawall for noticing ugliness.
>
> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
> ---
> drivers/input/tablet/gtco.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> Index: linux/drivers/input/tablet/gtco.c
> ===================================================================
> --- linux.orig/drivers/input/tablet/gtco.c
> +++ linux/drivers/input/tablet/gtco.c
> @@ -830,7 +830,7 @@ static int gtco_probe(struct usb_interfa
> struct gtco *gtco;
> struct input_dev *input_dev;
> struct hid_descriptor *hid_desc;
> - char *report = NULL;
> + char *report;
> int result = 0, retry;
> int error;
> struct usb_endpoint_descriptor *endpoint;
> @@ -916,12 +916,16 @@ static int gtco_probe(struct usb_interfa
> le16_to_cpu(hid_desc->wDescriptorLength),
> 5000); /* 5 secs */
>
> - if (result == le16_to_cpu(hid_desc->wDescriptorLength))
> + dbg("usb_control_msg result: %d", result);
> + if (result == le16_to_cpu(hid_desc->wDescriptorLength)) {
> + parse_hid_report_descriptor(gtco, report, result);
> break;
> + }
> }
>
> + kfree(report);
> +
> /* If we didn't get the report, fail */
> - dbg("usb_control_msg result: :%d", result);
> if (result != le16_to_cpu(hid_desc->wDescriptorLength)) {
> err("Failed to get HID Report Descriptor of size: %d",
> hid_desc->wDescriptorLength);
> @@ -929,12 +933,6 @@ static int gtco_probe(struct usb_interfa
> goto err_free_urb;
> }
>
> - /* Now we parse the report */
> - parse_hid_report_descriptor(gtco, report, result);
> -
> - /* Now we delete it */
> - kfree(report);
> -
> /* Create a device file node */
> usb_make_path(gtco->usbdev, gtco->usbpath, sizeof(gtco->usbpath));
> strlcat(gtco->usbpath, "/input0", sizeof(gtco->usbpath));
> @@ -988,7 +986,6 @@ static int gtco_probe(struct usb_interfa
> usb_buffer_free(gtco->usbdev, REPORT_MAX_SIZE,
> gtco->buffer, gtco->buf_dma);
> err_free_devs:
> - kfree(report);
> input_free_device(input_dev);
> kfree(gtco);
> return error;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
