ieee1394 incorrect unsigned tests
Quoted 49 lines · Julia Lawall
I found 63 occurrences of this problem with the following semantic match
(http://www.emn.fr/x-info/coccinelle/):@@ unsigned int i; @@
* i < 0
I looked through all of the results by hand, and they all seem to be problems. In many cases, it seems like the variable should not be unsigned as it is used to hold the return value of a function that might return a negative error code, but I haven't looked into this in detail.
diff -u -p
a/drivers/ieee1394/dv1394.cb/drivers/ieee1394/dv1394.c***a/drivers/ieee1394/dv1394.c2008-03-12 14:13:13.000000000 +0100(quoted diff) +0
-0@@ -918,7 +918,7 @@ static int do_dv1394_init(struct video_c /* default SYT offset is 3 cycles */ init->syt_offset = 3; * if ( (init->channel > 63) || (init->channel < 0) ) init->channel = 63; chan_mask = (u64)1 << init->channel;diff -u -p
a/drivers/ieee1394/video1394.cb/drivers/ieee1394/video1394.c***a/drivers/ieee1394/video1394.c2008-03-12 14:13:13.000000000 +0100(quoted diff) +0
-0@@ -893,7 +893,7 @@ static long video1394_ioctl(struct file if (unlikely(d = NULL)) return -EFAULT; * if (unlikely((v.buffer<0) || (v.buffer>=d->num_desc - 1))) { PRINT(KERN_ERR, ohci->host->id, "Buffer %d out of range",v.buffer); return -EINVAL;@@ -959,7 +959,7 @@ static long video1394_ioctl(struct file if (unlikely(d = NULL)) return -EFAULT; * if (unlikely((v.buffer<0) || (v.buffer>d->num_desc - 1))) { PRINT(KERN_ERR, ohci->host->id, "Buffer %d out of range",v.buffer); return -EINVAL;@@ -1030,7 +1030,7 @@ static long video1394_ioctl(struct file d = find_ctx(&ctx->context_list, OHCI_ISO_TRANSMIT, v.channel); if (d = NULL) return -EFAULT; * if ((v.buffer<0) || (v.buffer>=d->num_desc - 1)) { PRINT(KERN_ERR, ohci->host->id, "Buffer %d out of range",v.buffer); return -EINVAL;
Review static long video1394_ioctl(struct file · line 1143
Quoted 7 lines
(quoted diff) +0
-0@@ -1137,7 +1137,7 @@ static long video1394_ioctl(struct file d = find_ctx(&ctx->context_list, OHCI_ISO_TRANSMIT, v.channel); if (d = NULL) return -EFAULT; * if ((v.buffer<0) || (v.buffer>=d->num_desc-1)) { PRINT(KERN_ERR, ohci->host->id, "Buffer %d out of range",v.buffer); return -EINVAL;
I believe all these are essentially harmless. Due to the wrapping nature of unsigned arithmetic, while the first condition (x < 0) is always false, the condition it's attempting to catch will be caught by the second condition. We should probably remove the tests for < 0.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 7 lines · Matthew Wilcox
Quoted 6 lines · Julia Lawall
I found 63 occurrences of this problem with the following semantic match
(http://www.emn.fr/x-info/coccinelle/):@@ unsigned int i; @@
* i < 0
...
Quoted 40 lines
Quoted 40 lines
diff -u -p
a/drivers/ieee1394/dv1394.cb/drivers/ieee1394/dv1394.c***a/drivers/ieee1394/dv1394.c2008-03-12 14:13:13.000000000 +0100(quoted diff) +0
-0@@ -918,7 +918,7 @@ static int do_dv1394_init(struct video_c /* default SYT offset is 3 cycles */ init->syt_offset = 3; * if ( (init->channel > 63) || (init->channel < 0) ) init->channel = 63; chan_mask = (u64)1 << init->channel;diff -u -p
a/drivers/ieee1394/video1394.cb/drivers/ieee1394/video1394.c***a/drivers/ieee1394/video1394.c2008-03-12 14:13:13.000000000 +0100(quoted diff) +0
-0@@ -893,7 +893,7 @@ static long video1394_ioctl(struct file if (unlikely(d = NULL)) return -EFAULT; * if (unlikely((v.buffer<0) || (v.buffer>=d->num_desc - 1))) { PRINT(KERN_ERR, ohci->host->id, "Buffer %d out of range",v.buffer); return -EINVAL;@@ -959,7 +959,7 @@ static long video1394_ioctl(struct file if (unlikely(d = NULL)) return -EFAULT; * if (unlikely((v.buffer<0) || (v.buffer>d->num_desc - 1))) { PRINT(KERN_ERR, ohci->host->id, "Buffer %d out of range",v.buffer); return -EINVAL;@@ -1030,7 +1030,7 @@ static long video1394_ioctl(struct file d = find_ctx(&ctx->context_list, OHCI_ISO_TRANSMIT, v.channel); if (d = NULL) return -EFAULT; * if ((v.buffer<0) || (v.buffer>=d->num_desc - 1)) { PRINT(KERN_ERR, ohci->host->id, "Buffer %d out of range",v.buffer); return -EINVAL;@@ -1137,7 +1137,7 @@ static long video1394_ioctl(struct file d = find_ctx(&ctx->context_list, OHCI_ISO_TRANSMIT, v.channel); if (d = NULL) return -EFAULT;
Quoted 9 lines
Review static long video1394_ioctl(struct file · line 1143
Quoted 4 lines
(quoted diff) +0
-0* if ((v.buffer<0) || (v.buffer>=d->num_desc-1)) { PRINT(KERN_ERR, ohci->host->id, "Buffer %d out of range",v.buffer); return -EINVAL;I believe all these are essentially harmless. Due to the wrapping nature of unsigned arithmetic, while the first condition (x < 0) is always false, the condition it's attempting to catch will be caught by the second condition.
I agree.
Quoted 1 line
We should probably remove the tests for < 0.
We could. It also wouldn't hurt to leave the code as is, because gcc comes to the same conclusion as we do and removes these tests for us. (I checked with gcc 4.1.2 here.)
I gladly take patches, but there is no hurry because nobody works with this code anymore. Both dv1394 and video1394 are nowadays only touched if serious bugs are to be fixed, and dv1394 is deprecated anyway.
So the only apparent benefit of patching this would be to move these four sites out of the picture of scripted code checks.
Signature · 3 lines
Stefan Richter -===-=--- -=-- =--= http://arcgraph.de/sr/