Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

[Kevin Daughtridge]

On 09/19/12 01:05 N.U., Jiri Slaby wrote:
> AFAICS this is incorrect. Some drivers return pointers to their own 
> static structure from their .report_fixup. Hence there are two problems:
> * leak, because kmemdup'ped start is never freed
> * invalid free -- kfree(device->rdesc) will try to free a static 
> structure regards, 

On 09/19/12 04:55 N.U., Jiri Kosina wrote:
> How do you avoid memory leak on 'start' here? 

Hmm. I hadn't noticed that the other drivers are returning a static 
structure. In that case, it seems that report_fixup itself is broken 
from a memory perspective, in that it returns pointers to inconsistent 
storage types depending on the driver. (As evidenced by how my first 
patch version would have also caused invalid frees that weren't evident 
from the declaration in hid.h.) I see two options:

1. Ugly workaround: make a temporary copy of the dev_rdesc, give it to 
report_fixup, make a copy of the return, store that copy in rdesc, free 
the temporary copy. Though ugly, this would at least involve the 
smallest diff.

2. Standardize the behavior of the drivers' report_fixup 
implementations. Given that some of them need to change the size of the 
descriptor, modifying the passed structure is not an option. Probably 
all of them should return a newly allocated structure, either a modified 
copy of the input or a copy of their static, that can then be stored 
directly in rdesc. Especially since report_fixup is only ever called 
right before a copy is going to be taken anyway. (Adding constness to a 
parameter isn't considered a severe ABI break, is it?)

Thoughts?

-Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/