| Message ID | 20200822211839.5117-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: storage: initialize variable | expand |
On Sat, Aug 22, 2020 at 02:18:39PM -0700, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > clang static analysis reports this representative problem > > transport.c:495:15: warning: Assigned value is garbage or > undefined > length_left -= partial; > ^ ~~~~~~~ > partial is set only when usb_stor_bulk_transfer_sglist() > is successful. > > So initialize to partial to 0. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Tom Rix <trix@redhat.com> > --- > drivers/usb/storage/transport.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c > index 238a8088e17f..ce920851b1f2 100644 > --- a/drivers/usb/storage/transport.c > +++ b/drivers/usb/storage/transport.c > @@ -461,7 +461,7 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe, > int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe, > struct scsi_cmnd* srb) > { > - unsigned int partial; > + unsigned int partial = 0; > int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb), > scsi_sg_count(srb), scsi_bufflen(srb), > &partial); > @@ -484,7 +484,7 @@ int usb_stor_bulk_transfer_sg(struct us_data* us, unsigned int pipe, > void *buf, unsigned int length_left, int use_sg, int *residual) > { > int result; > - unsigned int partial; > + unsigned int partial = 0; > > /* are we scatter-gathering? */ > if (use_sg) { Yes, this is a bug. But the right way to fix it is to change usb_stor_bulk_transfer_sglist(): Make it store 0 to *act_len at the start. That way you change only one localized piece of code, instead of changing multiple callers and leaving a possibility of more errors being added in the future. Alan Stern
On 8/22/20 5:49 PM, Alan Stern wrote: > On Sat, Aug 22, 2020 at 02:18:39PM -0700, trix@redhat.com wrote: >> From: Tom Rix <trix@redhat.com> >> >> clang static analysis reports this representative problem >> >> transport.c:495:15: warning: Assigned value is garbage or >> undefined >> length_left -= partial; >> ^ ~~~~~~~ >> partial is set only when usb_stor_bulk_transfer_sglist() >> is successful. >> >> So initialize to partial to 0. >> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> drivers/usb/storage/transport.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c >> index 238a8088e17f..ce920851b1f2 100644 >> --- a/drivers/usb/storage/transport.c >> +++ b/drivers/usb/storage/transport.c >> @@ -461,7 +461,7 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe, >> int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe, >> struct scsi_cmnd* srb) >> { >> - unsigned int partial; >> + unsigned int partial = 0; >> int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb), >> scsi_sg_count(srb), scsi_bufflen(srb), >> &partial); >> @@ -484,7 +484,7 @@ int usb_stor_bulk_transfer_sg(struct us_data* us, unsigned int pipe, >> void *buf, unsigned int length_left, int use_sg, int *residual) >> { >> int result; >> - unsigned int partial; >> + unsigned int partial = 0; >> >> /* are we scatter-gathering? */ >> if (use_sg) { > Yes, this is a bug. But the right way to fix it is to change > usb_stor_bulk_transfer_sglist(): Make it store 0 to *act_len at the > start. > > That way you change only one localized piece of code, instead of > changing multiple callers and leaving a possibility of more errors being > added in the future. I'll respin the patch. It will exchange the compile time initialization fix for a run time if-check fix. > Alan Stern >
diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c index 238a8088e17f..ce920851b1f2 100644 --- a/drivers/usb/storage/transport.c +++ b/drivers/usb/storage/transport.c @@ -461,7 +461,7 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe, int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe, struct scsi_cmnd* srb) { - unsigned int partial; + unsigned int partial = 0; int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb), scsi_sg_count(srb), scsi_bufflen(srb), &partial); @@ -484,7 +484,7 @@ int usb_stor_bulk_transfer_sg(struct us_data* us, unsigned int pipe, void *buf, unsigned int length_left, int use_sg, int *residual) { int result; - unsigned int partial; + unsigned int partial = 0; /* are we scatter-gathering? */ if (use_sg) {