From b6c11191723d6eb884de5ee17d658298f5dd4127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 13 Jan 2017 12:02:04 +0100 Subject: [PATCH] [core] harden Glyph PBF parsing --- cmake/test-files.cmake | 1 + src/mbgl/text/glyph.hpp | 6 +- src/mbgl/text/glyph_atlas.cpp | 7 +- src/mbgl/text/glyph_pbf.cpp | 40 ++++++-- test/fixtures/resources/fake_glyphs-0-255.pbf | Bin 0 -> 432 bytes .../resources/fake_glyphs-0-255.pbf.txt | 86 ++++++++++++++++++ test/fixtures/resources/fake_glyphs.proto | 31 +++++++ test/text/glyph_pbf.test.cpp | 71 +++++++++++++++ 8 files changed, 230 insertions(+), 12 deletions(-) create mode 100644 test/fixtures/resources/fake_glyphs-0-255.pbf create mode 100644 test/fixtures/resources/fake_glyphs-0-255.pbf.txt create mode 100644 test/fixtures/resources/fake_glyphs.proto create mode 100644 test/text/glyph_pbf.test.cpp diff --git a/cmake/test-files.cmake b/cmake/test-files.cmake index 32ee23699..7eca71299 100644 --- a/cmake/test-files.cmake +++ b/cmake/test-files.cmake @@ -88,6 +88,7 @@ set(MBGL_TEST_FILES # text test/text/glyph_atlas.test.cpp + test/text/glyph_pbf.test.cpp test/text/quads.test.cpp # tile diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp index d07fbdff2..2bf144849 100644 --- a/src/mbgl/text/glyph.hpp +++ b/src/mbgl/text/glyph.hpp @@ -77,9 +77,13 @@ class Shaping { class SDFGlyph { public: + // We're using this value throughout the Mapbox GL ecosystem. If this is different, the glyphs + // also need to be reencoded. + static constexpr const uint8_t borderSize = 3; + uint32_t id = 0; - // A signed distance field of the glyph with a border of 3 pixels. + // A signed distance field of the glyph with a border (see above). std::string bitmap; // Glyph metrics diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp index 5d30dacdc..17b3e7e48 100644 --- a/src/mbgl/text/glyph_atlas.cpp +++ b/src/mbgl/text/glyph_atlas.cpp @@ -107,9 +107,6 @@ Rect GlyphAtlas::addGlyph(uintptr_t tileUID, const FontStack& fontStack, const SDFGlyph& glyph) { - // Use constant value for now. - const uint8_t buffer = 3; - std::map& face = index[fontStack]; auto it = face.find(glyph.id); @@ -125,8 +122,8 @@ Rect GlyphAtlas::addGlyph(uintptr_t tileUID, return Rect{ 0, 0, 0, 0 }; } - uint16_t buffered_width = glyph.metrics.width + buffer * 2; - uint16_t buffered_height = glyph.metrics.height + buffer * 2; + uint16_t buffered_width = glyph.metrics.width + SDFGlyph::borderSize * 2; + uint16_t buffered_height = glyph.metrics.height + SDFGlyph::borderSize * 2; // Add a 1px border around every image. const uint16_t padding = 1; diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp index 21edfe436..cdeac5798 100644 --- a/src/mbgl/text/glyph_pbf.cpp +++ b/src/mbgl/text/glyph_pbf.cpp @@ -12,9 +12,11 @@ #include +namespace mbgl { + namespace { -void parseGlyphPBF(mbgl::GlyphSet& glyphSet, const std::string& data) { +void parseGlyphPBF(GlyphSet& glyphSet, const GlyphRange& glyphRange, const std::string& data) { protozero::pbf_reader glyphs_pbf(data); while (glyphs_pbf.next(1)) { @@ -22,30 +24,39 @@ void parseGlyphPBF(mbgl::GlyphSet& glyphSet, const std::string& data) { while (fontstack_pbf.next(3)) { auto glyph_pbf = fontstack_pbf.get_message(); - mbgl::SDFGlyph glyph; + SDFGlyph glyph; + + bool hasID = false, hasWidth = false, hasHeight = false, hasLeft = false, + hasTop = false, hasAdvance = false; while (glyph_pbf.next()) { switch (glyph_pbf.tag()) { case 1: // id glyph.id = glyph_pbf.get_uint32(); + hasID = true; break; case 2: // bitmap glyph.bitmap = glyph_pbf.get_string(); break; case 3: // width glyph.metrics.width = glyph_pbf.get_uint32(); + hasWidth = true; break; case 4: // height glyph.metrics.height = glyph_pbf.get_uint32(); + hasHeight = true; break; case 5: // left glyph.metrics.left = glyph_pbf.get_sint32(); + hasLeft = true; break; case 6: // top glyph.metrics.top = glyph_pbf.get_sint32(); + hasTop = true; break; case 7: // advance glyph.metrics.advance = glyph_pbf.get_uint32(); + hasAdvance = true; break; default: glyph_pbf.skip(); @@ -53,15 +64,32 @@ void parseGlyphPBF(mbgl::GlyphSet& glyphSet, const std::string& data) { } } - glyphSet.insert(glyph.id, std::move(glyph)); + // If the area of width/height is non-zero, we need to adjust the expected size + // with the implicit border size, otherwise we expect there to be no bitmap at all. + const uint32_t expectedBitmapSize = + glyph.metrics.width && glyph.metrics.height + ? (glyph.metrics.width + 2 * SDFGlyph::borderSize) * + (glyph.metrics.height + 2 * SDFGlyph::borderSize) + : 0; + + // Only treat this glyph as a correct glyph if it has all required fields, and if + // the bitmap has the correct length. It also needs to satisfy a few metrics conditions + // that ensure that the glyph isn't bogus. All other glyphs are malformed. + // We're also discarding all glyphs that are outside the expected glyph range. + if (hasID && hasWidth && hasHeight && hasLeft && hasTop && hasAdvance && + glyph.metrics.width < 256 && glyph.metrics.height < 256 && + glyph.metrics.left >= -128 && glyph.metrics.left < 128 && + glyph.metrics.top >= -128 && glyph.metrics.top < 128 && + glyph.metrics.advance < 256 && glyph.bitmap.size() == expectedBitmapSize && + glyph.id >= glyphRange.first && glyph.id <= glyphRange.second) { + glyphSet.insert(glyph.id, std::move(glyph)); + } } } } } // namespace -namespace mbgl { - GlyphPBF::GlyphPBF(GlyphAtlas* atlas, const FontStack& fontStack, const GlyphRange& glyphRange, @@ -79,7 +107,7 @@ GlyphPBF::GlyphPBF(GlyphAtlas* atlas, observer->onGlyphsLoaded(fontStack, glyphRange); } else { try { - parseGlyphPBF(**atlas->getGlyphSet(fontStack), *res.data); + parseGlyphPBF(**atlas->getGlyphSet(fontStack), glyphRange, *res.data); } catch (...) { observer->onGlyphsError(fontStack, glyphRange, std::current_exception()); return; diff --git a/test/fixtures/resources/fake_glyphs-0-255.pbf b/test/fixtures/resources/fake_glyphs-0-255.pbf new file mode 100644 index 0000000000000000000000000000000000000000..8d7445d5c2c532b61f367deebb953aa9e5df3e57 GIT binary patch literal 432 zcmd;D%PghC;V8tCmYAI?@gD{l6&e~?H2&W_!fDWO>jBp4JJG#CsREEoXg8@rJJ literal 0 HcmV?d00001 diff --git a/test/fixtures/resources/fake_glyphs-0-255.pbf.txt b/test/fixtures/resources/fake_glyphs-0-255.pbf.txt new file mode 100644 index 000000000..6433af5d3 --- /dev/null +++ b/test/fixtures/resources/fake_glyphs-0-255.pbf.txt @@ -0,0 +1,86 @@ +# Make changes below, then run this command to update the encoded PBF file: +# protoc --encode glyphs test/fixtures/resources/fake_glyphs.proto < test/fixtures/resources/fake_glyphs-0-255.pbf.txt > test/fixtures/resources/fake_glyphs-0-255.pbf + +stacks { + # testing invalid metrics and bitmap too short + glyphs { + id: 65 + bitmap: "fake" + width: -1 + height: 65536 + left: -10000000 + top: 10000000 + advance: -42 + } + + # testing missing bitmap => malformed + glyphs { + id: 66 + width: 12 + height: 17 + left: 2 + top: -11 + advance: 15 + } + + # testing bitmap too long => malformed + glyphs { + id: 67 + bitmap: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + width: 1 + height: 1 + } + + # testing missing ID => malformed + glyphs { + width: 12 + height: 17 + left: 2 + top: -11 + advance: 15 + } + + # invalid metrics + bitmap => malformed + glyphs { + id: 65 + bitmap: "fake" + width: -1 + height: 65536 + left: -10000000 + top: 10000000 + advance: -42 + } + + # invalid metrics => malformed + glyphs { + id: 68 + bitmap: "x" + width: 1 + height: 1 + left: -10000000 + top: 10000000 + advance: -42 + } + + # a valid glyph + glyphs { + id: 69 + bitmap: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + width: 1 + height: 1 + left: 20 + top: 2 + advance: 8 + } + + # a valid glyph that has an ID outside the range + glyphs { + id: 300 + bitmap: "" + width: 0 + height: 0 + left: 0 + top: 0 + advance: 0 + } +} diff --git a/test/fixtures/resources/fake_glyphs.proto b/test/fixtures/resources/fake_glyphs.proto new file mode 100644 index 000000000..d074541d6 --- /dev/null +++ b/test/fixtures/resources/fake_glyphs.proto @@ -0,0 +1,31 @@ +syntax = "proto2"; + +option optimize_for = LITE_RUNTIME; + +// Stores a glyph with metrics and optional SDF bitmap information. +message glyph { + optional int64 id = 1; + + // A signed distance field of the glyph with a border of 3 pixels. + optional bytes bitmap = 2; + + // Glyph metrics. + optional int64 width = 3; + optional int64 height = 4; + optional sint64 left = 5; + optional sint64 top = 6; + optional int64 advance = 7; +} + +// Stores fontstack information and a list of faces. +message fontstack { + optional string name = 1; + optional string range = 2; + repeated glyph glyphs = 3; +} + +message glyphs { + repeated fontstack stacks = 1; + + extensions 16 to 8191; +} diff --git a/test/text/glyph_pbf.test.cpp b/test/text/glyph_pbf.test.cpp new file mode 100644 index 000000000..1e28dfbc3 --- /dev/null +++ b/test/text/glyph_pbf.test.cpp @@ -0,0 +1,71 @@ +#include + +#include +#include +#include +#include +#include +#include +#include + +#include + +using namespace mbgl; + +using namespace std::string_literals; + +class MockGlyphAtlasObserver : public GlyphAtlasObserver { +public: + std::function glyphsLoaded; + std::function glyphsError; + + void onGlyphsLoaded(const FontStack& fontStack, const GlyphRange& glyphRange) override { + if (glyphsLoaded) { + glyphsLoaded(fontStack, glyphRange); + } + } + void onGlyphsError(const FontStack& fontStack, const GlyphRange& glyphRange, std::exception_ptr error) override { + if (glyphsError) { + glyphsError(fontStack, glyphRange, error); + } + } +}; + +TEST(GlyphPBF, Parsing) { + util::RunLoop loop; + DefaultFileSource fileSource{ ":memory:", "." }; + GlyphAtlas glyphAtlas{ { 1024, 1024 }, fileSource }; + FontStack fontStack{ "fake_glyphs" }; + GlyphRange glyphRange{ 0, 255 }; + + glyphAtlas.setURL("asset://test/fixtures/resources/{fontstack}-{range}.pbf"); + + MockGlyphAtlasObserver glyphAtlasObserver; + glyphAtlasObserver.glyphsLoaded = [&](const FontStack&, const GlyphRange&) { + loop.stop(); + + auto sdfs = glyphAtlas.getGlyphSet(fontStack)->getSDFs(); + + // The fake glyphs don't contain a glyph that has the ID 0; it only contains glyphs with + // undefined IDs, but the parser should remove them. + + EXPECT_TRUE(sdfs.size() == 1); + EXPECT_TRUE(sdfs.find(69) != sdfs.end()); + auto& sdf = sdfs[69]; + EXPECT_EQ("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"s, sdf.bitmap); + EXPECT_EQ(1u, sdf.metrics.width); + EXPECT_EQ(1u, sdf.metrics.height); + EXPECT_EQ(20, sdf.metrics.left); + EXPECT_EQ(2, sdf.metrics.top); + EXPECT_EQ(8u, sdf.metrics.advance); + }; + + glyphAtlasObserver.glyphsError = [&](const FontStack&, const GlyphRange&, std::exception_ptr error) { + loop.stop(); + FAIL() << util::toString(error); + }; + + GlyphPBF pbf(&glyphAtlas, fontStack, glyphRange, &glyphAtlasObserver, fileSource); + + loop.run(); +}