I'm trying to cross check implementations of the same interface for
errors. I assume that if functions are assigned to the same function
pointer, they are implementations of a common interface and should be
consistent with each other. For example, if one implementation checks one
of its arguments for NULL, the other implementation should also check that
argument for NULL.
In this case, I'm looking at which arguments are referenced at all in the
implementation. If we have 10 implementations and 9 of them read argument
1 and the 10th fails to read argument 1, the 10th implementation may be
missing some code. In each of the reports below, I give an example
implementation that does read the argument. That is followed by an
implementation that fails to read the argument.
If you find yourself CC'd on this email, it is because I think that you
are the maintainer of code that contains one of the error reports. Search
for your email address to find the appropriate report.
This checker is experimental, so be warned that these reports may be
false. I would very much appreciate your feedback and suggestions so that
I can give you better reports in the future.
Thanks,
Ken Ashcraft
# Total = 8
# BUGs | File Name
1 | /drivers/scsi/scsi_sysfs.c
1 | /drivers/net/wan/dscc4.c
1 | /drivers/usb/serial/ipaq.c
1 | /drivers/ieee1394/eth1394.c
1 | /drivers/usb/storage/scsiglue.c
1 | /drivers/net/wan/hdlc_cisco.c
1 | /drivers/block/paride/bpck6.c
1 | /drivers/scsi/ide-scsi.c
---------------------------------------------------------
[BUG] ([email protected]) looks like it should return count
instead of strlen(buf), but this is in scsiglue.c, so is it special code?
example:
/home/kash/linux/2.6.3/linux-2.6.3/drivers/scsi/scsi_sysfs.c:274:store_rescan_field:
NOTE:READ: Checking arg count [EXAMPLE=device_attribute.store-2]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/storage/scsiglue.c:321:store_max_sectors:
ERROR:READ: Not checking arg [COUNTER=device_attribute.store-2] [fit=1]
[fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=233] [counter=1] [z =
3.20943839741638] [fn-z = -4.35889894354067]
return sprintf(buf, "%u\n", sdev->request_queue->max_sectors);
}
/* Input routine for the sysfs max_sectors file */
Error --->
static ssize_t store_max_sectors(struct device *dev, const char *buf,
size_t count)
{
struct scsi_device *sdev = to_scsi_device(dev);
---------------------------------------------------------
[BUG] <[email protected]> not doing anything with buf
example:
/home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/core/driverfs.c:56:set_bConfigurationValue:
NOTE:READ: Checking arg buf [EXAMPLE=device_attribute.store-1]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/scsi/scsi_sysfs.c:271:store_rescan_field:
ERROR:READ: Not checking arg [COUNTER=device_attribute.store-1] [SMALL]
[fit=2] [fit_fn=2] [fn_ex=0] [fn_counter=1] [ex=232] [counter=2] [z =
2.90949088363915] [fn-z = -4.35889894354067]
sdev_rd_attr (model, "%.16s\n");
sdev_rd_attr (rev, "%.4s\n");
sdev_rw_attr_bit (online);
static ssize_t
Error --->
store_rescan_field (struct device *dev, const char *buf, size_t count)
{
scsi_rescan_device(dev);
return count;
---------------------------------------------------------
[BUG] ([email protected]) unimplemented
example:
/home/kash/linux/2.6.3/linux-2.6.3/drivers/ieee1394/eth1394.c:304:ether1394_tx_timeout:
NOTE:READ: Checking arg dev [EXAMPLE=net_device.tx_timeout-0]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/wan/dscc4.c:957:dscc4_tx_timeout:
ERROR:READ: Not checking arg [COUNTER=net_device.tx_timeout-0] [SMALL]
[fit=3] [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=84] [counter=1] [z =
1.61743595582868] [fn-z = -4.35889894354067]
done:
dpriv->timer.expires = jiffies + TX_TIMEOUT;
add_timer(&dpriv->timer);
}
Error --->
static void dscc4_tx_timeout(struct net_device *dev)
{
/* FIXME: something is missing there */
}
---------------------------------------------------------
[BUG] <[email protected]> doesn't look at 'capacity'
example:
/home/kash/linux/2.6.3/linux-2.6.3/drivers/message/i2o/i2o_scsi.c:1046:i2o_scsi_bios_param:
NOTE:READ: Checking arg capacity [EXAMPLE=Scsi_Host_Template.bios_param-2]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/scsi/ide-scsi.c:917:idescsi_bios:
ERROR:READ: Not checking arg [COUNTER=Scsi_Host_Template.bios_param-2]
[fit=12] [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=25] [counter=1] [z =
0.26995276239951] [fn-z = -4.35889894354067]
/* finally, reset the drive (and its partner on the bus...) */
ide_do_reset (drive);
return SUCCESS;
}
Error --->
static int idescsi_bios(struct scsi_device *sdev, struct block_device *bdev,
sector_t capacity, int *parm)
{
idescsi_scsi_t *idescsi = scsihost_to_idescsi(sdev->host);
---------------------------------------------------------
[BUG] ([email protected]) I got a lot of reports from
bpck6.c. I don't know if it just manages to do things completely
differently from the other drivers or if it is wrong. Here is a place
where they don't read 'pi->delay'
example:
/home/kash/linux/2.6.3/linux-2.6.3/drivers/block/paride/aten.c:76:aten_read_block:
NOTE:READ: Checking arg (*pi).delay
[EXAMPLE=pi_protocol.read_block-0.deref.delay]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/block/paride/bpck6.c:90:bpck6_read_block:
ERROR:READ: Not checking arg
[COUNTER=pi_protocol.read_block-0.deref.delay] [SMALL] [fit=41] [fit_fn=1]
[fn_ex=0] [fn_counter=1] [ex=14] [counter=1] [z = -0.296174438879545]
[fn-z = -4.35889894354067]
static void bpck6_write_block( PIA *pi, char * buf, int len )
{
ppc6_wr_port16_blk(PPCSTRUCT(pi),ATAPI_DATA,buf,(u32)len>>1);
}
Error --->
static void bpck6_read_block( PIA *pi, char * buf, int len )
{
ppc6_rd_port16_blk(PPCSTRUCT(pi),ATAPI_DATA,buf,(u32)len>>1);
}
---------------------------------------------------------
[BUG] <[email protected]> why not set the mac addr?
example:
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/acenic.c:3113:ace_set_mac_addr:
NOTE:READ: Checking arg p [EXAMPLE=net_device.set_mac_address-1]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/ieee1394/eth1394.c:647:ether1394_mac_addr:
ERROR:READ: Not checking arg [COUNTER=net_device.set_mac_address-1]
[SMALL] [fit=52] [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=28] [counter=2]
[z = -0.418853908291694] [fn-z = -4.35889894354067]
unsigned char * haddr)
{
memcpy(((u8*)hh->hh_data) + 6, haddr, dev->addr_len);
}
Error --->
static int ether1394_mac_addr(struct net_device *dev, void *p)
{
if (netif_running(dev))
return -EBUSY;
---------------------------------------------------------
[BUG] <[email protected]> not referencing 'dev'
example:
/home/kash/linux/2.6.3/linux-2.6.3/drivers/ieee1394/eth1394.c:569:ether1394_header:
NOTE:READ: Checking arg dev [EXAMPLE=net_device.hard_header-1]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/net/wan/hdlc_cisco.c:37:cisco_hard_header:
ERROR:READ: Not checking arg [COUNTER=net_device.hard_header-1] [fit=62]
[fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=10] [counter=1] [z =
-0.622543017479467] [fn-z = -4.35889894354067]
#define CISCO_ADDR_REQ 0 /* Cisco address request */
#define CISCO_ADDR_REPLY 1 /* Cisco address reply */
#define CISCO_KEEPALIVE_REQ 2 /* Cisco keepalive request */
Error --->
static int cisco_hard_header(struct sk_buff *skb, struct net_device *dev,
u16 type, void *daddr, void *saddr,
unsigned int len)
{
---------------------------------------------------------
[BUG] <[email protected]> doesn't reference 'port->serial->dev'
example:
/home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/serial/cyberjack.c:280:cyberjack_write:
NOTE:READ: Checking arg (*serial).dev
[EXAMPLE=usb_serial_device_type.write-0.deref.serial.deref.dev]
/home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/serial/ipaq.c:368:ipaq_write:
ERROR:READ: Not checking arg
[COUNTER=usb_serial_device_type.write-0.deref.serial.deref.dev] [fit=202]
[fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=12] [counter=2] [z =
-1.59416228266091] [fn-z = -4.35889894354067]
if (result)
err("%s - failed resubmitting read urb, error %d", __FUNCTION__, result);
return;
}
Error --->
static int ipaq_write(struct usb_serial_port *port, int from_user, const
unsigned char *buf,
int count)
{
const unsigned char *current_position = buf;
On Tue, Apr 27, 2004 at 02:47:22PM -0700, Ken Ashcraft wrote:
> I'm trying to cross check implementations of the same interface for
> errors. I assume that if functions are assigned to the same function
> pointer, they are implementations of a common interface and should be
> consistent with each other. For example, if one implementation checks one
> of its arguments for NULL, the other implementation should also check that
> argument for NULL.
>
> In this case, I'm looking at which arguments are referenced at all in the
> implementation. If we have 10 implementations and 9 of them read argument
> 1 and the 10th fails to read argument 1, the 10th implementation may be
> missing some code. In each of the reports below, I give an example
> implementation that does read the argument. That is followed by an
> implementation that fails to read the argument.
>
> ---------------------------------------------------------
> [BUG] ([email protected]) looks like it should return count
> instead of strlen(buf), but this is in scsiglue.c, so is it special code?
>
> example:
> /home/kash/linux/2.6.3/linux-2.6.3/drivers/scsi/scsi_sysfs.c:274:store_rescan_field:
> NOTE:READ: Checking arg count [EXAMPLE=device_attribute.store-2]
>
> /home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/storage/scsiglue.c:321:store_max_sectors:
> ERROR:READ: Not checking arg [COUNTER=device_attribute.store-2] [fit=1]
> [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=233] [counter=1] [z =
> 3.20943839741638] [fn-z = -4.35889894354067]
>
> return sprintf(buf, "%u\n", sdev->request_queue->max_sectors);
> }
>
> /* Input routine for the sysfs max_sectors file */
>
> Error --->
> static ssize_t store_max_sectors(struct device *dev, const char *buf,
> size_t count)
> {
> struct scsi_device *sdev = to_scsi_device(dev);
My understanding was that I was supposed to return the number of bytes in
the buffer that I actually used. I thought 'count' was the maximum size I
could use.
Is that not the case?
Matt
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
Oh great modem, why hast thou forsaken me?
-- Dust Puppy
User Friendly, 3/2/1998
> On Tue, Apr 27, 2004 at 02:47:22PM -0700, Ken Ashcraft wrote:
>> I'm trying to cross check implementations of the same interface for
>> errors. I assume that if functions are assigned to the same function
>> pointer, they are implementations of a common interface and should be
>> consistent with each other. For example, if one implementation checks
>> one
>> of its arguments for NULL, the other implementation should also check
>> that
>> argument for NULL.
>>
>> In this case, I'm looking at which arguments are referenced at all in
>> the
>> implementation. If we have 10 implementations and 9 of them read
>> argument
>> 1 and the 10th fails to read argument 1, the 10th implementation may be
>> missing some code. In each of the reports below, I give an example
>> implementation that does read the argument. That is followed by an
>> implementation that fails to read the argument.
>>
>
>> ---------------------------------------------------------
>> [BUG] ([email protected]) looks like it should return count
>> instead of strlen(buf), but this is in scsiglue.c, so is it special
>> code?
>>
>> example:
>> /home/kash/linux/2.6.3/linux-2.6.3/drivers/scsi/scsi_sysfs.c:274:store_rescan_field:
>> NOTE:READ: Checking arg count [EXAMPLE=device_attribute.store-2]
>>
>> /home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/storage/scsiglue.c:321:store_max_sectors:
>> ERROR:READ: Not checking arg [COUNTER=device_attribute.store-2] [fit=1]
>> [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=233] [counter=1] [z >
>> 3.20943839741638] [fn-z = -4.35889894354067]
>>
>> return sprintf(buf, "%u\n", sdev->request_queue->max_sectors);
>> }
>>
>> /* Input routine for the sysfs max_sectors file */
>>
>> Error --->
>> static ssize_t store_max_sectors(struct device *dev, const char *buf,
>> size_t count)
>> {
>> struct scsi_device *sdev = to_scsi_device(dev);
>
> My understanding was that I was supposed to return the number of bytes in
> the buffer that I actually used. I thought 'count' was the maximum size I
> could use.
>
That sounds feasible. I can't find any documentation on the interface, so
I can't tell. However, there are ~233 functions that at least reference
count. Many of them are almost identical: they call sscanf or strtol to
get a length out of buf, pass that length to some subroutine, and then
unconditionally return count. This is a more representative example than
the one listed above.
static ssize_t set_pwm_enable1(struct device *dev, const char *buf,
size_t count)
{
struct i2c_client *client = to_i2c_client(dev);
struct asb100_data *data = i2c_get_clientdata(client);
unsigned long val = simple_strtoul(buf, NULL, 10);
data->pwm &= 0x0f; /* keep the duty cycle bits */
data->pwm |= (val ? 0x80 : 0x00);
asb100_write_value(client, ASB100_REG_PWM1, data->pwm);
return count;
}
On Tue, Apr 27, 2004 at 03:58:33PM -0700, Ken Ashcraft wrote:
> > On Tue, Apr 27, 2004 at 02:47:22PM -0700, Ken Ashcraft wrote:
> >
> >> ---------------------------------------------------------
> >> [BUG] ([email protected]) looks like it should return count
> >> instead of strlen(buf), but this is in scsiglue.c, so is it special
> >> code?
> >>
> >> example:
> >> /home/kash/linux/2.6.3/linux-2.6.3/drivers/scsi/scsi_sysfs.c:274:store_rescan_field:
> >> NOTE:READ: Checking arg count [EXAMPLE=device_attribute.store-2]
> >>
> >> /home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/storage/scsiglue.c:321:store_max_sectors:
> >> ERROR:READ: Not checking arg [COUNTER=device_attribute.store-2] [fit=1]
> >> [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=233] [counter=1] [z >
> >> 3.20943839741638] [fn-z = -4.35889894354067]
> >>
> >> return sprintf(buf, "%u\n", sdev->request_queue->max_sectors);
> >> }
> >>
> >> /* Input routine for the sysfs max_sectors file */
> >>
> >> Error --->
> >> static ssize_t store_max_sectors(struct device *dev, const char *buf,
> >> size_t count)
> >> {
> >> struct scsi_device *sdev = to_scsi_device(dev);
> >
> > My understanding was that I was supposed to return the number of bytes in
> > the buffer that I actually used. I thought 'count' was the maximum size I
> > could use.
> >
>
> That sounds feasible. I can't find any documentation on the interface, so
> I can't tell. However, there are ~233 functions that at least reference
> count. Many of them are almost identical: they call sscanf or strtol to
> get a length out of buf, pass that length to some subroutine, and then
> unconditionally return count. This is a more representative example than
> the one listed above.
>
> static ssize_t set_pwm_enable1(struct device *dev, const char *buf,
> size_t count)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct asb100_data *data = i2c_get_clientdata(client);
> unsigned long val = simple_strtoul(buf, NULL, 10);
> data->pwm &= 0x0f; /* keep the duty cycle bits */
> data->pwm |= (val ? 0x80 : 0x00);
> asb100_write_value(client, ASB100_REG_PWM1, data->pwm);
> return count;
> }
It seems pretty shady to me to have a function which is passed a value, the
entire purpose of which is to return it to the caller unchanged.
Perhaps someone on l-k can comment...
Matt
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
Hey Chief. We've figured out how to save the technical department. We
need to be committed.
-- The Techs
User Friendly, 1/22/1998
On Tue, 27 Apr 2004, Matthew Dharm wrote:
> On Tue, Apr 27, 2004 at 03:58:33PM -0700, Ken Ashcraft wrote:
> > > On Tue, Apr 27, 2004 at 02:47:22PM -0700, Ken Ashcraft wrote:
> > >
> > >> ---------------------------------------------------------
> > >> [BUG] ([email protected]) looks like it should return count
> > >> instead of strlen(buf), but this is in scsiglue.c, so is it special
> > >> code?
> > >>
> > >> example:
> > >> /home/kash/linux/2.6.3/linux-2.6.3/drivers/scsi/scsi_sysfs.c:274:store_rescan_field:
> > >> NOTE:READ: Checking arg count [EXAMPLE=device_attribute.store-2]
> > >>
> > >> /home/kash/linux/2.6.3/linux-2.6.3/drivers/usb/storage/scsiglue.c:321:store_max_sectors:
> > >> ERROR:READ: Not checking arg [COUNTER=device_attribute.store-2] [fit=1]
> > >> [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=233] [counter=1] [z >
> > >> 3.20943839741638] [fn-z = -4.35889894354067]
> > >>
> > >> return sprintf(buf, "%u\n", sdev->request_queue->max_sectors);
> > >> }
> > >>
> > >> /* Input routine for the sysfs max_sectors file */
> > >>
> > >> Error --->
> > >> static ssize_t store_max_sectors(struct device *dev, const char *buf,
> > >> size_t count)
> > >> {
> > >> struct scsi_device *sdev = to_scsi_device(dev);
> > >
> > > My understanding was that I was supposed to return the number of bytes in
> > > the buffer that I actually used. I thought 'count' was the maximum size I
> > > could use.
> > >
> >
> > That sounds feasible. I can't find any documentation on the interface, so
> > I can't tell. However, there are ~233 functions that at least reference
> > count. Many of them are almost identical: they call sscanf or strtol to
> > get a length out of buf, pass that length to some subroutine, and then
> > unconditionally return count. This is a more representative example than
> > the one listed above.
> >
> > static ssize_t set_pwm_enable1(struct device *dev, const char *buf,
> > size_t count)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct asb100_data *data = i2c_get_clientdata(client);
> > unsigned long val = simple_strtoul(buf, NULL, 10);
> > data->pwm &= 0x0f; /* keep the duty cycle bits */
> > data->pwm |= (val ? 0x80 : 0x00);
> > asb100_write_value(client, ASB100_REG_PWM1, data->pwm);
> > return count;
> > }
>
> It seems pretty shady to me to have a function which is passed a value, the
> entire purpose of which is to return it to the caller unchanged.
>
> Perhaps someone on l-k can comment...
>
> Matt
There are many funtions that are called for their effect (like printf),
and many functions that return the same value that they were passed,
like memcpy(), memmove(), strcpy(), strncpy(), etc.
In the subject code, the effect of the function was to condition
some parameters and call another function, returning the same
value it was passed. Sounds good to me.
Cheers,
Dick Johnson
Penguin : Linux version 2.4.26 on an i686 machine (5557.45 BogoMips).
Note 96.31% of all statistics are fiction.
"Ken Ashcraft" <[email protected]> writes:
/drivers/net/wan/hdlc_cisco.c:
> ---------------------------------------------------------
> [BUG] <[email protected]> not referencing 'dev'
>
> example:
> /home/kash/linux/2.6.3/linux-2.6.3/drivers/ieee1394/eth1394.c:569:ether1394_header:
> NOTE:READ: Checking arg dev [EXAMPLE=net_device.hard_header-1]
>
> /home/kash/linux/2.6.3/linux-2.6.3/drivers/net/wan/hdlc_cisco.c:37:cisco_hard_header:
> ERROR:READ: Not checking arg [COUNTER=net_device.hard_header-1] [fit=62]
> [fit_fn=1] [fn_ex=0] [fn_counter=1] [ex=10] [counter=1] [z =
> -0.622543017479467] [fn-z = -4.35889894354067]
> #define CISCO_ADDR_REQ 0 /* Cisco address request */
> #define CISCO_ADDR_REPLY 1 /* Cisco address reply */
> #define CISCO_KEEPALIVE_REQ 2 /* Cisco keepalive request */
>
>
>
> Error --->
> static int cisco_hard_header(struct sk_buff *skb, struct net_device *dev,
> u16 type, void *daddr, void *saddr,
> unsigned int len)
> {
False positive - while other hard_header functions may want to know
the actual outbound device, it isn't the case here.
--
Krzysztof Halasa, B*FH