On Monday 23 July 2012 11:25:25 Lukasz Majewski wrote:
Dear Mike Frysinger,
On Tuesday 03 July 2012 05:38:05 Lukasz Majewski wrote:
+{
- int ret;
- static char str[16];
- memset(str, '\0', sizeof(str));
- strncpy(str, shortname, sizeof(shortname));
no need for the memset.
The gadget can be called from many separate commands (e.g. command "dfu" and command "ums") and those commands can be executed without power cycle. Thereof I need to be sure, that str is not polluted by previous name.
that makes no sense. please read the documentation of the str*cpy functions -- they do no analysis of the target string and merely copy the source to the destination. thus this code is basically:
str[0] = '\0'; str[1] = '\0'; str[...] = '\0'; str[0] = shortname[0]; str[1] = shortname[1]; str[...] = shortname[...];
it should be fairly obvious now why that memset is pointless.
this strncpy looks broken -- the 3rd arg is for how many bytes are available in the *dest* buffer, not how long the source is.
After looking deeply into the source I admit that providing the upper bound on the dest is more safe.
it isn't a matter of being safe, it's a matter of correctness
strncat(str, s, sizeof(str));this is also incorrect. the length given to strncat is how many bytes are left, not the total length.
I cannot agree. sizeof(str) return 16, which is the destination buffer size.
which is wrong. please read the strncat specification.
since this string parsing logic is all just completely broken, i'd suggest replacing it all with:
{ int ret; /* We only allow "dfu" atm, so 3 should be enough */ static char name[sizeof(shortname) + 3];
if (strcmp(s, "dfu")) { printf("%s: unknown command: %s\n", __func__, s); return -EINVAL; }
strcpy(name, shortname); strcat(name, s);
This is a very neat design, but it assumes that there will be only one function ("dfu" in this case). For this particular function +3 applies, but what if another function (like "usb_storage") will be defined?
why does that matter ? the snippet i posted above is trivial to extend to support any number of functions. increase the "3" to the max you care about, and then add more strcmp() to the if statement.
I'm now working on another function - the USB Mass Storage (named "ums" ;-) ).
Another issue is omitting the strncmp/strncpy functions and depending on the: static char name[sizeof(shortname) + 3]; definition to prevent buffer overflow.
your existing code is already full of bugs that don't prevent overflow, and having the "3" right next to the "dfu" with a comment makes it pretty clear what is going on. -mike