Re: [PATCH] scsi: ses fix for len and mem leaking when fail to add intf

["Yinghai Lu" <yhlu.kernel@gmail.com>]

On Feb 9, 2008 7:00 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Sat, 2008-02-09 at 04:13 -0800, Yinghai Lu wrote:
> > [PATCH] scsi: ses fix for len and mem leaking when fail to add intf
> >
> > change to u32 before left shifting char
>
> This one is a bit unnecessary; C promotion rules guarantee that
> everything is promoted to int (or above) before doing arithmetic.  Since
> it's only ever done on 16 bits, signed or unsigned int is adequate for
> the conversion.

thank. just learned that.

yhlu@yhlunb:~/xx/xx/notes> cat ctest.c
#include <stdio.h>
int main(int argc, char *argv[])
{
        unsigned char buf[20];
        int len;

        buf[2] = 0x02;
        buf[3] = 0x03;

        len = (buf[2] << 8) + buf[3];

        printf("len = %x\n", len);
        return 0;
}
yhlu@yhlunb:~/xx/xx/notes> gcc -o ctest ctest.c
yhlu@yhlunb:~/xx/xx/notes> ./ctest
len = 203
yhlu@yhlunb:~/xx/xx/notes>


>
> > also fix leaking with scomp leaking when failing.
>
> Yes, I see that, thanks!  There's also the kmalloc of scomp which should
> be kzalloc if you care to fix that up in the resend.
>
> > -             edev =  enclosure_find(&sdev->host->shost_gendev);
> > +             edev = enclosure_find(&sdev->host->shost_gendev);
>
> Space cleanups also need mention in the changelog.
>
> > -     ses_dev->page1 = buf;
> > -     ses_dev->page1_len = len;
> > -
> >       result = ses_recv_diag(sdev, 1, buf, len);
> >       if (result)
> >               goto recv_failed;
> >
> > +     ses_dev->page1 = buf;
> > +     ses_dev->page1_len = len;
> > +
>
> Neither of us gets this right.  By removing the kfree(buf) from the
> err_free path, you cause a leak here.  I cause a double free.  I think
> putting back the kfree(buf) and keeping this hunk is the fix.

the buf already become sdev->page1, sdev->pag10, sdev->page2.
so it will be freed via them

>
> >       types = buf[10];
> >       len = buf[11];
> >
> > @@ -474,11 +474,12 @@ static int ses_intf_add(struct class_dev
> >                       components += type_ptr[1];
> >       }
> >
> > +     buf = NULL;
>
> Yes, prevents double free (but only if buf is freed).

it became sdev->page1 already

>
> >       result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
> >       if (result)
> >               goto recv_failed;
> >
> > @@ -492,11 +493,12 @@ static int ses_intf_add(struct class_dev
> >
> >       /* The additional information page --- allows us
> >        * to match up the devices */
> > +     buf = NULL;
>
> It's probably better to move these closer to the statements that make
> them necessary (in this case above the comment).

OK

>
> >       if (IS_ERR(edev)) {
> >               err = PTR_ERR(edev);
> > +             kfree(scomp);
> >               goto err_free;
> >       }
>
> kfree(scomp) should be in the err_free path just in case someone else
> adds something to this.

ok.

>
> >       /* add 1 for trailing '\0' we'll use */
> >       buf = kzalloc(len + 1, GFP_KERNEL);
> > -     result = ses_recv_diag(sdev, 7, buf, len);
> > -     if (result) {
> > +     if (buf)
> > +             result = ses_recv_diag(sdev, 7, buf, len);
> > +     else
> > +             result = 7;
> > +
>
> What exactly is this supposed to be doing, and why 7?  If you're
> thinking of conditioning the page 7 receive on the success of the
> allocation, we really need the allocation failure report more than we
> need the driver to attach.

want to move out label out of if later.

>
> > -                                     addl_desc_ptr += addl_desc_ptr[1] + 2;
> > +                                     addl_desc_ptr += 2 + addl_desc_ptr[1];
>
> This is rather pointless, isn't it?
>
> >   err_free:
> > -     kfree(buf);
>
> You can't remove this.  Also add kfree(scomp) here.