On 22 Aug 2009, George Spelvin wrote:
> > Yes, this is a bug. In ascii2utf, this line:
> >
> > if (utfmax > 0) {
> >
> > should be replaced with this:
> >
> > if (*s && utfmax > 0) {
>
> Ah, I see. Now it makes sense what the function is trying to do.
> How about the following simplification instead:
> (Released to the public domain, copyright abandoned, have fun...)
>
> /*
> * helper routine for returning string descriptors in UTF-16LE
> * input can actually be ISO-8859-1; ASCII is its 7-bit subset.
> * @s: Null-terminated ASCII (actually ISO-8859-1) string
> * @utf: Buffer for UTF-16LE output
> * @utfmax: Length (in bytes; may be odd) of utf buffer.
> *
> * The return value is the number of bytes filled in: 2*strlen(s) or
> * utfmax, whichever is less.
> */
> static unsigned ascii2utf(char const *s, u8 *utf, unsigned utfmax)
> {
> u8 * const utf0 = utf;
>
> while (*s && utfmax--) {
> *utf++ = *s++;
> if (!utfmax--)
> break;
> *utf++ = 0;
> }
> return (unsigned)(utf - utf0);
> }
>
> You could also re-jigger rh_string() to just pass the cd->self.bus_name
> and hcd->product_desc strings to ascii2utf() by reference rather than
> strlcpy()ing them.
>
> I also was worried that rh_string would leak data from the uninitialized
> stack buffer if id > 3. It turns out that that can't happen, but it sure
> got my heart rate going for a bit...
>
> static unsigned
> rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
> {
> char buf[100];
> char const *s;
> unsigned slen;
>
> // language ids
> switch (id) {
> case 0:
> /* Array of LANGID codes (0x0409 is MSFT-speak for "en-us") */
> static char const langids[4] = {4, 3, 0x09, 0x04};
> if (len > 4)
> len = 4;
> memcpy(data, langids, len);
> return len;
> case 1:
> /* Serial number */
> s = hcd->self.bus_name;
> break;
> case 2:
> /* Product name */
> s = hcd->product_desc;
> break;
> case 3:
> /* Manufacturer */
> snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
> init_utsname()->release, hcd->driver->description);
> s = buf;
> break;
> default:
> /* Can't happen; caller guarantees it */
> return 0;
> }
>
> slen = strlen(s);
> if (slen > 126) /* Should never happen, but make sure */
> slen = 126;
>
> switch (len) { /* All cases fall through */
> default:
> len = 2 + ascii2utf(s, data + 2, len - 2);
> case 2:
> data [1] = 3; /* Descriptor type == string */
> case 1:
> data [0] = 2 * (slen + 1); /* Total descriptor length */
> case 0:
> ; /* Compiler wants a statement here */
> }
> return len;
> }
>
>
>
> You could also refactor the code to fill in the first 2 bytes inside
> ascii2utf(). That cleans things up a lot:
>
> /*
> * helper routine for returning string descriptors in UTF-16LE
> * input can actually be ISO-8859-1; ASCII is its 7-bit subset.
> * @s: Null-terminated ASCII (actually ISO-8859-1) string
> * @utf: Buffer for USB string descritpro (header + UTF-16LE)
> * @utfmax: Length (in bytes; may be odd) of utf buffer.
> *
> * The return value is the number of bytes filled in: 2 + 2*strlen(s) or
> * utfmax, whichever is less.
> */
> static unsigned
> ascii2desc(char const *s, u8 *utf, unsigned utfmax)
> {
> u8 * const utf0 = utf;
> unsigned slen = strlen(s);
>
> if (slen > 126)
> slen = 126; /* Limit so 2 + 2*slen <= 255 */
>
> if (!utfmax--)
> goto end;
> *utf++ = 2 + 2*slen; /* Descriptor total length */
>
> if (!utfmax--)
> goto end;
> *utf++ = 3; /* Descriptor type 3 (string) */
>
> if (utfmax > 2*slen)
> utfmax = 2*slen;
>
> while (utfmax--) {
> *utf++ = *s++;
> if (!utfmax--)
> break;
> *utf++ = 0;
> }
> end:
> return (unsigned)(utf - utf0);
> }
>
> static unsigned
> rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
> {
> char buf[100];
> char const *s;
>
> // language ids
> switch (id) {
> case 0:
> /* Array of LANGID codes (0x0409 is MSFT-speak for "en-us") */
> static char const langids[4] = {4, 3, 0x09, 0x04};
> if (len > 4)
> len = 4;
> memcpy(data, langids, len);
> return len;
> case 1:
> /* Serial number */
> s = hcd->self.bus_name;
> break;
> case 2:
> /* Product name */
> s = hcd->product_desc;
> break;
> case 3:
> /* Manufacturer */
> snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
> init_utsname()->release, hcd->driver->description);
> s = buf;
> break;
> default:
> /* Can't happen; caller guarantees it */
> return 0;
> }
>
> return ascii2desc(s, data, len);
> }
>
>
> That sure looks a lot cleaner to me...
Please feel free to submit a patch.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
