type mismatch

type mismatch

[Julia Lawall]

I came across the following code
(drivers/gpu/drm/nouveau/nvkm/subdev/i2c/pad.h):

struct nvkm_i2c_pad {
        struct nvkm_object base;
        int index;
        struct nvkm_i2c_port *port;
        struct nvkm_i2c_port *next;
};

static inline struct nvkm_i2c_pad *
nvkm_i2c_pad(struct nvkm_i2c_port *port)
{
 	struct nvkm_object *pad = nv_object(port);
	while (!nv_iclass(pad->parent, NV_SUBDEV_CLASS))
		pad = pad->parent;
	return (void *)pad;
}

Would it be better for the return to use container_of, rather than a cast
that happens to work because of the position of the nvkm_object field?

julia

Re: type mismatch

[Dan Carpenter]

I don't really have a strong opinion either way...  It's unlikely that
we will introduce a bug here and if we did, I think it would be caught
immediately in testing.

It's pretty common to treat the first member of a struct as special.
What annoys me is when people do

struct foo {
	int one, two, three;
	whatever;
};

memcpy(&foo.one, src, sizoef(struct foo));

Argh!?  These triger buffer overflows warnings in Smatch and I don't
see the point since &foo.one is less readable than &foo!  Oh well, I
think these were common enough, I had to treat it as idiomatic and add a
special case for them.

regards,
dan carpenter

Re: type mismatch

[Julia Lawall]

On Sun, 10 May 2015, Dan Carpenter wrote:

> I don't really have a strong opinion either way...  It's unlikely that
> we will introduce a bug here and if we did, I think it would be caught
> immediately in testing.
> 
> It's pretty common to treat the first member of a struct as special.
> What annoys me is when people do
> 
> struct foo {
> 	int one, two, three;
> 	whatever;
> };
> 
> memcpy(&foo.one, src, sizoef(struct foo));
> 
> Argh!?  These triger buffer overflows warnings in Smatch and I don't
> see the point since &foo.one is less readable than &foo!  Oh well, I
> think these were common enough, I had to treat it as idiomatic and add a
> special case for them.

OK, thanks.

julia

Re: type mismatch

[Nicholas Mc Guire]

On Sun, 10 May 2015, Dan Carpenter wrote:

> I don't really have a strong opinion either way...  It's unlikely that
> we will introduce a bug here and if we did, I think it would be caught
> immediately in testing.
> 
> It's pretty common to treat the first member of a struct as special.
> What annoys me is when people do
> 
> struct foo {
> 	int one, two, three;
> 	whatever;
> };
> 
> memcpy(&foo.one, src, sizoef(struct foo));
> 
> Argh!?  These triger buffer overflows warnings in Smatch and I don't
> see the point since &foo.one is less readable than &foo!  Oh well, I
> think these were common enough, I had to treat it as idiomatic and add a
> special case for them.
>
just ran a naive scanner for that pattern but only could find this one instance

./net/sctp/sm_make_chunk.c:3103 ugly memset 
3102          if (af->is_any(&addr))
3103                  memcpy(&addr.v4, sctp_source(asconf), sizeof(addr));

my scanner ist:

@uses_first@
identifier f;
idexpression s;
identifier e;
position p;                                                                     
@@

f(...){
<+...
* memcpy@p(&s.e,...,sizeof(s));
...+>
}

@script:python@
p << uses_first.p;
@@
print "%s:%s ugly memset" % (p[0].file,p[0].line)

so is my scanner broken/incomplete or has this pattern lost its popularity ?

thx!
hofrat

Re: type mismatch

[Julia Lawall]

On Mon, 11 May 2015, Nicholas Mc Guire wrote:

> On Sun, 10 May 2015, Dan Carpenter wrote:
> 
> > I don't really have a strong opinion either way...  It's unlikely that
> > we will introduce a bug here and if we did, I think it would be caught
> > immediately in testing.
> > 
> > It's pretty common to treat the first member of a struct as special.
> > What annoys me is when people do
> > 
> > struct foo {
> > 	int one, two, three;
> > 	whatever;
> > };
> > 
> > memcpy(&foo.one, src, sizoef(struct foo));
> > 
> > Argh!?  These triger buffer overflows warnings in Smatch and I don't
> > see the point since &foo.one is less readable than &foo!  Oh well, I
> > think these were common enough, I had to treat it as idiomatic and add a
> > special case for them.
> >
> just ran a naive scanner for that pattern but only could find this one instance
> 
> ./net/sctp/sm_make_chunk.c:3103 ugly memset 
> 3102          if (af->is_any(&addr))
> 3103                  memcpy(&addr.v4, sctp_source(asconf), sizeof(addr));
> 
> my scanner ist:
> 
> @uses_first@
> identifier f;
> idexpression s;

Maybe generalize s (ie expression) and the size (ie, don't specify 
anything)?  The sizeof could be a type also.

julia

> identifier e;
> position p;                                                                     
> @@
> 
> f(...){
> <+...
> * memcpy@p(&s.e,...,sizeof(s));
> ...+>
> }
> 
> @script:python@
> p << uses_first.p;
> @@
> print "%s:%s ugly memset" % (p[0].file,p[0].line)
> 
> so is my scanner broken/incomplete or has this pattern lost its popularity ?
> 
> thx!
> hofrat
> 

Re: type mismatch

[Dan Carpenter]

Hm...  You're right.  I'm going to retract some things.

1) I didn't end up adding a special case handling for this, I only
   wanted to but never got around to it.

2) Also these aren't *that* common as I was thinking.  They're mostly
   limitted to emulex.

drivers/net/ethernet/emulex/benet/be_cmds.c
  3173  
  3174          be_wrb_cmd_hdr_prepare(&req->hdr, CMD_SUBSYSTEM_COMMON,
                                       ^^^^^^^^^
First element.

  3175                                 OPCODE_COMMON_SET_HSW_CONFIG, sizeof(*req), wrb,
                                                                     ^^^^^^^^^^^^
Here is the sizeof().

  3176                                 NULL);
  3177  

3) It's also obvious why emulex would do this because the req struct
   could be defined a lot of different ways so long as the first element
   is a header.  It's like a union.

Sorry for the misinformation.  My bad.

regards,
dan carpenter

Re: type mismatch

[Nicholas Mc Guire]

On Mon, 11 May 2015, Julia Lawall wrote:

> 
> 
> On Mon, 11 May 2015, Nicholas Mc Guire wrote:
> 
> > On Sun, 10 May 2015, Dan Carpenter wrote:
> > 
> > > I don't really have a strong opinion either way...  It's unlikely that
> > > we will introduce a bug here and if we did, I think it would be caught
> > > immediately in testing.
> > > 
> > > It's pretty common to treat the first member of a struct as special.
> > > What annoys me is when people do
> > > 
> > > struct foo {
> > > 	int one, two, three;
> > > 	whatever;
> > > };
> > > 
> > > memcpy(&foo.one, src, sizoef(struct foo));
> > > 
> > > Argh!?  These triger buffer overflows warnings in Smatch and I don't
> > > see the point since &foo.one is less readable than &foo!  Oh well, I
> > > think these were common enough, I had to treat it as idiomatic and add a
> > > special case for them.
> > >
> > just ran a naive scanner for that pattern but only could find this one instance
> > 
> > ./net/sctp/sm_make_chunk.c:3103 ugly memset 
> > 3102          if (af->is_any(&addr))
> > 3103                  memcpy(&addr.v4, sctp_source(asconf), sizeof(addr));
> > 
> > my scanner ist:
> > 
> > @uses_first@
> > identifier f;
> > idexpression s;
> 
> Maybe generalize s (ie expression) and the size (ie, don't specify 
> anything)?  The sizeof could be a type also.
>
yup - adding the type check case finds two additional cases
but generalizing s to an expression did not change results

@uses_first@
identifier f;
type T;
idexpression T s;
identifier e;
position p;
@@

f(...){
<+...
(
* memcpy@p(&s.e,...,sizeof(s));
|
* memcpy@p(&s.e,...,sizeof(T));
)
...+>
}

./drivers/infiniband/hw/ehca/ehca_mcast.c:80 ugly memset
./drivers/infiniband/hw/ehca/ehca_mcast.c:117 ugly memset
./net/sctp/sm_make_chunk.c:3103 ugly memset

thx!
hofrat