| Message ID | 20230122154901.505142-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] paride/pcd: return earlier when an error happens in pcd_atapi() | expand |
On 1/22/23 8:49 AM, Tom Rix wrote: > clang static analysis reports > drivers/block/paride/pcd.c:856:36: warning: The left operand of '&' > is a garbage value [core.UndefinedBinaryOperatorResult] > tocentry->cdte_ctrl = buffer[5] & 0xf; > ~~~~~~~~~ ^ Has this one been compiled? I'm guessing not tested... In any case, this code is going away hopefully shortly, so let's not bother with changes like this.
On 1/22/23 12:49 PM, Jens Axboe wrote: > On 1/22/23 8:49 AM, Tom Rix wrote: >> clang static analysis reports >> drivers/block/paride/pcd.c:856:36: warning: The left operand of '&' >> is a garbage value [core.UndefinedBinaryOperatorResult] >> tocentry->cdte_ctrl = buffer[5] & 0xf; >> ~~~~~~~~~ ^ > Has this one been compiled? I'm guessing not tested... > > In any case, this code is going away hopefully shortly, so let's not > bother with changes like this. Going away soon would be nice, this is an old problem. I did not bother with a fixes: tag because it was is when the repo was created in 2005. Tom >
On Sun, Jan 22, 2023 at 01:49:00PM -0700, Jens Axboe wrote: > On 1/22/23 8:49 AM, Tom Rix wrote: > > clang static analysis reports > > drivers/block/paride/pcd.c:856:36: warning: The left operand of '&' > > is a garbage value [core.UndefinedBinaryOperatorResult] > > tocentry->cdte_ctrl = buffer[5] & 0xf; > > ~~~~~~~~~ ^ > > Has this one been compiled? I'm guessing not tested... > > In any case, this code is going away hopefully shortly, so let's not > bother with changes like this. Look at the callers - the value left in entry is discarded if ->audio_ioctl(..., CDROMREADTOCENTRY, &entry) returns non-zero. Sure, it's a nasal daemon territory, but realistically it's not going to be caught by testing.
On 1/22/23 3:10 PM, Al Viro wrote: > On Sun, Jan 22, 2023 at 01:49:00PM -0700, Jens Axboe wrote: >> On 1/22/23 8:49 AM, Tom Rix wrote: >>> clang static analysis reports >>> drivers/block/paride/pcd.c:856:36: warning: The left operand of '&' >>> is a garbage value [core.UndefinedBinaryOperatorResult] >>> tocentry->cdte_ctrl = buffer[5] & 0xf; >>> ~~~~~~~~~ ^ >> >> Has this one been compiled? I'm guessing not tested... >> >> In any case, this code is going away hopefully shortly, so let's not >> bother with changes like this. > > Look at the callers - the value left in entry is discarded if > ->audio_ioctl(..., CDROMREADTOCENTRY, &entry) returns non-zero. Sure, > it's a nasal daemon territory, but realistically it's not going to be > caught by testing. I don't expect anyone really to be able to test it, but v1 had a pretty basic issue that would've surely triggered a compiler warning had it been compiled.
diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c index a5ab40784119..47757ba1a09f 100644 --- a/drivers/block/paride/pcd.c +++ b/drivers/block/paride/pcd.c @@ -825,14 +825,14 @@ static int pcd_audio_ioctl(struct cdrom_device_info *cdi, unsigned int cmd, void struct cdrom_tochdr *tochdr = (struct cdrom_tochdr *) arg; char buffer[32]; - int r; - r = pcd_atapi(cd, cmd, 12, buffer, "read toc header"); + if (pcd_atapi(cd, cmd, 12, buffer, "read toc header")) + return -EIO; tochdr->cdth_trk0 = buffer[2]; tochdr->cdth_trk1 = buffer[3]; - return r ? -EIO : 0; + return 0; } case CDROMREADTOCENTRY: @@ -845,13 +845,13 @@ static int pcd_audio_ioctl(struct cdrom_device_info *cdi, unsigned int cmd, void struct cdrom_tocentry *tocentry = (struct cdrom_tocentry *) arg; unsigned char buffer[32]; - int r; cmd[1] = (tocentry->cdte_format == CDROM_MSF ? 0x02 : 0); cmd[6] = tocentry->cdte_track; - r = pcd_atapi(cd, cmd, 12, buffer, "read toc entry"); + if (pcd_atapi(cd, cmd, 12, buffer, "read toc entry")) + return -EIO; tocentry->cdte_ctrl = buffer[5] & 0xf; tocentry->cdte_adr = buffer[5] >> 4; @@ -866,7 +866,7 @@ static int pcd_audio_ioctl(struct cdrom_device_info *cdi, unsigned int cmd, void (((((buffer[8] << 8) + buffer[9]) << 8) + buffer[10]) << 8) + buffer[11]; - return r ? -EIO : 0; + return 0; } default:
clang static analysis reports drivers/block/paride/pcd.c:856:36: warning: The left operand of '&' is a garbage value [core.UndefinedBinaryOperatorResult] tocentry->cdte_ctrl = buffer[5] & 0xf; ~~~~~~~~~ ^ When the call to pcd_atapi() fails, buffer[] is in an unknown state, so return early. Signed-off-by: Tom Rix <trix@redhat.com> --- v2: remove unused 'r' variable --- drivers/block/paride/pcd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)