[webkit-reviews] review denied: [Bug 28131] [Haiku] Adding font-specific files to WebCore. : [Attachment 34894] Adding four font-specific files to WebCore.

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Maxime Simon
<simon.maxime at gmail.com>'s request for review:
Bug 28131: [Haiku] Adding font-specific files to WebCore.
https://bugs.webkit.org/show_bug.cgi?id=28131

Attachment 34894: Adding four font-specific files to WebCore.
https://bugs.webkit.org/attachment.cgi?id=34894&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
> +static font_family* findMatchingFontFamily(const AtomicString& familyName)
> +{
> +    font_family family;
> +
> +    if (BFont().SetFamilyAndStyle(BString(familyName.string()).String(), 0)
== B_OK)
> +	   strcpy(family, BString(familyName.string()).String());
Not sure where familyName comes from, but i suspect it's from CSS, and that
means you have a buffer overflow vulnerability here.  Never use strcpy --
always strncpy.  That said, wth is a font_family?  From reading this it looks
like it's just a string?

> +    else {
> +	   // If no font family is found for the given name, we use a generic
font.
> +	   if (BString(familyName.string()).FindFirst("Sans") != B_ERROR)
> +	       strcpy(family, "DejaVu Sans");
> +	   else if (BString(familyName.string()).FindFirst("Serif") != B_ERROR)


> +	       strcpy(family, "DejaVu Serif");
> +	   else if (BString(familyName.string()).FindFirst("Mono") != B_ERROR)
> +	       strcpy(family, "DejaVu Mono");
> +	   else // This is the fallback font.
> +	       strcpy(family, "DejaVu Serif");
This code seems to repeatedly create the same bstring, which seems
unnecessarily expensive.  More over i'm fairly sure AtomicString has all the
logic you need to do this without ever creating a bstring anyway.

> +    }
> +
> +    font_family* fontFamily = &family;
> +    return fontFamily;
You can't return a pointer to a stack allocated variable, it's just not okay --

the code structure implies that the compiler even warned you.

> +}
> +
> +static font_style* findMatchingFontStyle(font_family family, bool bold, bool

oblique)
> +{
> +    font_style style;
...
> +    font_style* fontStyle = &style;
> +    return fontStyle;
Once again, don't return pointers to locals -- you're far better off just using

out parameters