[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