From 80e756daefb3b2fe0df3f9182f3c674924bd53c9 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Mon, 2 Oct 2023 17:48:46 +0100 Subject: [PATCH] LibGfx: Load BitmapFont data more safely Previously, `load_from_memory()` just took a raw pointer to the data, and then manually calculated offsets from that pointer. Instead, let's use the MappedFile we already have, to stream in the data, to make things a bit safer. We also now check that the entire file's data was read, since if there was data left over, then either the file is bad or we've done something wrong. I've moved the code directly into `try_load_from_mapped_file()` since `load_from_memory()` was only called from there. The extra indirection wasn't adding anything. --- Userland/Libraries/LibGfx/Font/BitmapFont.cpp | 48 +++++++++++++------ Userland/Libraries/LibGfx/Font/BitmapFont.h | 4 +- 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/Userland/Libraries/LibGfx/Font/BitmapFont.cpp b/Userland/Libraries/LibGfx/Font/BitmapFont.cpp index 745762368c..ebaa4f26ad 100644 --- a/Userland/Libraries/LibGfx/Font/BitmapFont.cpp +++ b/Userland/Libraries/LibGfx/Font/BitmapFont.cpp @@ -40,6 +40,18 @@ static_assert(AssertSize()); static constexpr size_t s_max_glyph_count = 0x110000; static constexpr size_t s_max_range_mask_size = s_max_glyph_count / (256 * 8); +} + +// FIXME: We define the traits for the const FontFileHeader, because that's the one we use, and defining +// Traits doesn't apply to Traits. Once that's fixed, remove the const here. +template<> +class AK::Traits : public GenericTraits { +public: + static constexpr bool is_trivially_serializable() { return true; } +}; + +namespace Gfx { + NonnullRefPtr BitmapFont::clone() const { return MUST(try_clone()); @@ -176,9 +188,9 @@ BitmapFont::~BitmapFont() } } -ErrorOr> BitmapFont::load_from_memory(u8 const* data) +ErrorOr> BitmapFont::try_load_from_mapped_file(NonnullOwnPtr mapped_file) { - auto const& header = *reinterpret_cast(data); + auto& header = *TRY(mapped_file->read_in_place()); if (memcmp(header.magic, "!Fnt", 4)) return Error::from_string_literal("Gfx::BitmapFont::load_from_memory: Incompatible header"); if (header.name[sizeof(header.name) - 1] != '\0') @@ -188,16 +200,29 @@ ErrorOr> BitmapFont::load_from_memory(u8 const* data) size_t bytes_per_glyph = sizeof(u32) * header.glyph_height; size_t glyph_count { 0 }; - u8* range_mask_start = const_cast(data + sizeof(FontFileHeader)); - Bytes range_mask { range_mask_start, header.range_mask_size }; + + // FIXME: These ReadonlyFoo -> Foo casts are awkward, and only needed because BitmapFont is + // sometimes editable and sometimes not. Splitting it into editable/non-editable classes + // would make this a lot cleaner. + ReadonlyBytes readonly_range_mask = TRY(mapped_file->read_in_place(header.range_mask_size)); + Bytes range_mask { const_cast(readonly_range_mask.data()), readonly_range_mask.size() }; for (size_t i = 0; i < header.range_mask_size; ++i) glyph_count += 256 * popcount(range_mask[i]); - u8* rows_start = range_mask_start + header.range_mask_size; - Bytes rows { rows_start, glyph_count * bytes_per_glyph }; - Span widths { rows_start + glyph_count * bytes_per_glyph, glyph_count }; + + ReadonlyBytes readonly_rows = TRY(mapped_file->read_in_place(glyph_count * bytes_per_glyph)); + Bytes rows { const_cast(readonly_rows.data()), readonly_rows.size() }; + + ReadonlySpan readonly_widths = TRY(mapped_file->read_in_place(glyph_count)); + Span widths { const_cast(readonly_widths.data()), readonly_widths.size() }; + + if (!mapped_file->is_eof()) + return Error::from_string_literal("Gfx::BitmapFont::load_from_memory: Trailing data in file"); + auto name = TRY(String::from_utf8(ReadonlyBytes { header.name, strlen(header.name) })); auto family = TRY(String::from_utf8(ReadonlyBytes { header.family, strlen(header.family) })); - return adopt_nonnull_ref_or_enomem(new (nothrow) BitmapFont(move(name), move(family), rows, widths, !header.is_variable_width, header.glyph_width, header.glyph_height, header.glyph_spacing, range_mask, header.baseline, header.mean_line, header.presentation_size, header.weight, header.slope)); + auto font = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) BitmapFont(move(name), move(family), rows, widths, !header.is_variable_width, header.glyph_width, header.glyph_height, header.glyph_spacing, range_mask, header.baseline, header.mean_line, header.presentation_size, header.weight, header.slope))); + font->m_mapped_file = move(mapped_file); + return font; } RefPtr BitmapFont::load_from_file(DeprecatedString const& path) @@ -211,13 +236,6 @@ ErrorOr> BitmapFont::try_load_from_file(DeprecatedStri return try_load_from_mapped_file(move(mapped_file)); } -ErrorOr> BitmapFont::try_load_from_mapped_file(OwnPtr mapped_file) -{ - auto font = TRY(load_from_memory((u8 const*)mapped_file->data())); - font->m_mapped_file = move(mapped_file); - return font; -} - ErrorOr BitmapFont::write_to_file(DeprecatedString const& path) { auto stream = TRY(Core::File::open(path, Core::File::OpenMode::Write)); diff --git a/Userland/Libraries/LibGfx/Font/BitmapFont.h b/Userland/Libraries/LibGfx/Font/BitmapFont.h index dc937166e4..d7893cb157 100644 --- a/Userland/Libraries/LibGfx/Font/BitmapFont.h +++ b/Userland/Libraries/LibGfx/Font/BitmapFont.h @@ -31,7 +31,7 @@ public: static RefPtr load_from_file(DeprecatedString const& path); static ErrorOr> try_load_from_file(DeprecatedString const& path); - static ErrorOr> try_load_from_mapped_file(OwnPtr); + static ErrorOr> try_load_from_mapped_file(NonnullOwnPtr); ErrorOr write_to_file(DeprecatedString const& path); ErrorOr write_to_file(NonnullOwnPtr file); @@ -134,8 +134,6 @@ private: u8 glyph_width, u8 glyph_height, u8 glyph_spacing, Bytes range_mask, u8 baseline, u8 mean_line, u8 presentation_size, u16 weight, u8 slope, bool owns_arrays = false); - static ErrorOr> load_from_memory(u8 const*); - template int unicode_view_width(T const& view) const;