[webkit-reviews] review denied: [Bug 31106] [Chromium] handle web fonts in a secure manner : [Attachment 42691] transcode_webfonts_by_ots_v6

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Nov 8 22:27:25 PST 2009


David Levin <levin at chromium.org> has denied Yusuke Sato
<yusukes at chromium.org>'s request for review:
Bug 31106: [Chromium] handle web fonts in a secure manner
https://bugs.webkit.org/show_bug.cgi?id=31106

Attachment 42691: transcode_webfonts_by_ots_v6
https://bugs.webkit.org/attachment.cgi?id=42691&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-11-07  Yusuke Sato  <yusukes at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   handle web fonts in a secure manner
> +	   https://bugs.webkit.org/show_bug.cgi?id=31106
> +
> +	   Add support for OpenType sanitiser (OTS). This is experimental code
that is
> +	   Chromium only for the moment.

It isn't for chromium only anymore.


> diff --git a/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp
b/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp

>  #include "FontPlatformData.h"
>  #include "NotImplemented.h"
> +#if ENABLE(OPENTYPE_SANITIZER)
> +#include "OpenTypeSanitizer.h"
> +#endif

The "if enable" should be in the header and then the include should be be done
with no "if enable".

>  #include "SharedBuffer.h"

>  
>  #if PLATFORM(WIN_OS)
> @@ -245,6 +248,14 @@ FontCustomPlatformData*
createFontCustomPlatformData(SharedBuffer* buffer)
>  {
>      ASSERT_ARG(buffer, buffer);
>  
> +#if ENABLE(OPENTYPE_SANITIZER)
> +    OpenTypeSanitizer sanitizer(buffer);
> +    PassRefPtr<SharedBuffer> transcodeBuffer = sanitizer.sanitize();

Use RefPtr not PassRefPtr in functons.

> +    if (!transcodeBuffer)
> +	   return 0;  // validation failed.

One space before end of line comments.

> +    buffer = transcodeBuffer.get();
> +#endif

> diff --git a/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp
b/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp

> +#if ENABLE(OPENTYPE_SANITIZER)
> +    OpenTypeSanitizer sanitizer(buffer);
> +    PassRefPtr<SharedBuffer> transcodeBuffer = sanitizer.sanitize();

Use RefPtr not PassRefPtr in functons.

> +    if (!transcodeBuffer)
> +	   return 0;  // validation failed.

One space before end of line comments.

> +    buffer = transcodeBuffer.get();
> +#endif

> diff --git a/WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp
b/WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (c) 2009, Google Inc. All rights reserved.

Use a capital c
no comma after the year

"Copyright (C) 2009 Google Inc. All rights reserved."


Add "#if ENABLE(OPENTYPE_SANITIZER)" in the file right before "#include
"config.h"". Put the endif if at end of the file "#endif //
ENABLE(OPENTYPE_SANITIZER)"

> +#include "config.h"

> +namespace WebCore {
> +
> +PassRefPtr<SharedBuffer> OpenTypeSanitizer::sanitize()
> +{
> +    if (!m_buffer)
> +	   return 0;
> +
> +#if PLATFORM(CHROMIUM)

I know you did this if PLATFORM at Eric's suggest, but I think it turned out in
a form that no one would use.

So I would just get rid of *all* "if PLATFORM(CHROMIUM)" in this file. If
others want to use it in the future, it can be adjusted then to meet their
needs.


> +    // This is the largest web font size which we'll try to transcode.
> +    static const size_t maxWebFontSize = 30 * 1024 * 1024;  // 30 MB

One space before end of line comments.

> +    if (m_buffer->size() > maxWebFontSize)
> +	   return 0;
> +
> +    // A transcoded font is usually smaller than an original font.
> +    // However, it can be slightly bigger than the original one due to
> +    // name table replacement and/or padding for glyf table.

I've typically seen glyph instead of glyf but I did see glyf in one place on
the web.

> +    static const size_t padLen = 20 * 1024;	// 20kB

One space before end of line comments.

> +
> +    unsigned char* transcodeRawBuffer = new unsigned char[m_buffer->size() +
padLen];

Use OwnArrayPtr.

> +    ots::MemoryStream output(transcodeRawBuffer, m_buffer->size() + padLen);

> +    if (!ots::Process(&output, (const uint8_t *) m_buffer->data(),
m_buffer->size())) {

Use c++ style cast. (reinterpret_cast). instead of "(const uint8_t *)".

> +	   delete[] transcodeRawBuffer;

This goes away when you use OwnArrayPtr (and then the "if" will have one line
so the {} will go away on the if clause.)

> +	   return 0;
> +    }

> +    const size_t transcodeLen = output.Tell();
> +    return SharedBuffer::create(transcodeRawBuffer, transcodeLen);

I think this leaks but once you switch to OwnArrayPtr, it won't.

> diff --git a/WebCore/platform/graphics/opentype/OpenTypeSanitizer.h
b/WebCore/platform/graphics/opentype/OpenTypeSanitizer.h
> + * Copyright (c) 2009, Google Inc. All rights reserved.

Use a capital c
no comma after the year

> +#ifndef OpenTypeSanitizer_h
> +#define OpenTypeSanitizer_h

Add the if enable here.


More information about the webkit-reviews mailing list