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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
