On Mon, Jun 27, 2022 at 05:35:24PM +0800, Zhang Boyang wrote:
> This patch adds an argument 'scale' to grub_font_draw_glyph(). If
> scale > 1, then the function will create a new scaled bitmap of the
> drawing glyph, and draws the scaled glyph. The scaled bitmap is cached
> in the glyph itself, so it can be reused if same glyph is used many
> times.
>
> To avoid interger overflow during scaling, this patch intruduces
> dimension limits to font files, described by GRUB_FONT_MAX_DIMENSION.
> The scaled glyph should also obey this limit. In addition, scale factor
> is also limited by GRUB_FONT_MAX_SCALE.
If you use overflow aware math operators defined as macros, e.g.
grub_mul(), defined in include/grub/safemath.h probably you
can drop these artificial limits.
> Signed-off-by: Zhang Boyang <zhangboyang...@gmail.com>
> ---
> grub-core/commands/videotest.c | 4 +-
> grub-core/font/font.c | 120 ++++++++++++++++++++++++++++++---
> grub-core/gfxmenu/font.c | 2 +-
> grub-core/term/gfxterm.c | 2 +-
> include/grub/font.h | 16 ++++-
> 5 files changed, 131 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/commands/videotest.c b/grub-core/commands/videotest.c
> index ac145afc2..d95ee411d 100644
> --- a/grub-core/commands/videotest.c
> +++ b/grub-core/commands/videotest.c
> @@ -87,7 +87,7 @@ grub_cmd_videotest (grub_command_t cmd __attribute__
> ((unused)),
> return grub_error (GRUB_ERR_BAD_FONT, "no font loaded");
>
> glyph = grub_font_get_glyph (fixed, '*');
> - grub_font_draw_glyph (glyph, color, 200 ,0);
> + grub_font_draw_glyph (glyph, color, 200, 0, 1);
>
> color = grub_video_map_rgb (255, 255, 255);
>
> @@ -148,7 +148,7 @@ grub_cmd_videotest (grub_command_t cmd __attribute__
> ((unused)),
> {
> color = grub_video_map_color (i);
> palette[i] = color;
> - grub_font_draw_glyph (glyph, color, 16 + i * 16, 220);
> + grub_font_draw_glyph (glyph, color, 16 + i * 16, 220, 1);
> }
> }
>
> diff --git a/grub-core/font/font.c b/grub-core/font/font.c
> index 42189c325..46ffec84c 100644
> --- a/grub-core/font/font.c
> +++ b/grub-core/font/font.c
> @@ -137,6 +137,8 @@ ascii_glyph_lookup (grub_uint32_t code)
> ascii_font_glyph[current]->offset_x = 0;
> ascii_font_glyph[current]->offset_y = -2;
> ascii_font_glyph[current]->device_width = 8;
> + ascii_font_glyph[current]->scaled_bitmap = NULL;
> + ascii_font_glyph[current]->scale = 0;
> ascii_font_glyph[current]->font = NULL;
>
> grub_memcpy (ascii_font_glyph[current]->bitmap,
> @@ -172,6 +174,8 @@ grub_font_loader_init (void)
> unknown_glyph->offset_x = 0;
> unknown_glyph->offset_y = -3;
> unknown_glyph->device_width = 8;
> + unknown_glyph->scaled_bitmap = NULL;
> + unknown_glyph->scale = 0;
> grub_memcpy (unknown_glyph->bitmap,
> unknown_glyph_bitmap, sizeof (unknown_glyph_bitmap));
>
> @@ -646,6 +650,20 @@ grub_font_load (const char *filename)
> goto fail;
> }
>
> + if (font->max_char_width <= 0
> + || font->max_char_width > GRUB_FONT_MAX_DIMENSION
> + || font->max_char_height <= 0
> + || font->max_char_height > GRUB_FONT_MAX_DIMENSION
> + || font->ascent <= 0
> + || font->ascent > GRUB_FONT_MAX_DIMENSION
> + || font->descent <= 0
> + || font->descent > GRUB_FONT_MAX_DIMENSION)
> + {
> + grub_error (GRUB_ERR_BAD_FONT,
> + "invalid font file: bad font metrics");
grub_error (GRUB_ERR_BAD_FONT, N_("invalid font file: bad font metrics"));
Every grub_error() message has to be processed by N_() macro. And yes,
there is no space after N_ and before "(".
Additionally, I am OK with lines a bit longer than 80 chars. So, you can
put grub_error() call in one line here and below.
> + goto fail;
> + }
> +
> /* Add the font to the global font registry. */
> if (register_font (font) != 0)
> goto fail;
> @@ -738,7 +756,7 @@ grub_font_get_glyph_internal (grub_font_t font,
> grub_uint32_t code)
> grub_uint16_t height;
> grub_int16_t xoff;
> grub_int16_t yoff;
> - grub_int16_t dwidth;
> + grub_uint16_t dwidth;
> int len;
>
> if (index_entry->glyph)
> @@ -760,7 +778,18 @@ grub_font_get_glyph_internal (grub_font_t font,
> grub_uint32_t code)
> || read_be_uint16 (font->file, &height) != 0
> || read_be_int16 (font->file, &xoff) != 0
> || read_be_int16 (font->file, &yoff) != 0
> - || read_be_int16 (font->file, &dwidth) != 0)
> + || read_be_uint16 (font->file, &dwidth) != 0)
> + {
> + remove_font (font);
> + return 0;
> + }
> +
> + /* Reject illegal glyphs. */
> + if (width > font->max_char_width
> + || height > font->max_char_height
> + || xoff < -GRUB_FONT_MAX_DIMENSION || xoff > GRUB_FONT_MAX_DIMENSION
> + || yoff < -GRUB_FONT_MAX_DIMENSION || yoff > GRUB_FONT_MAX_DIMENSION
> + || dwidth > GRUB_FONT_MAX_DIMENSION)
> {
> remove_font (font);
> return 0;
> @@ -780,6 +809,8 @@ grub_font_get_glyph_internal (grub_font_t font,
> grub_uint32_t code)
> glyph->offset_x = xoff;
> glyph->offset_y = yoff;
> glyph->device_width = dwidth;
> + glyph->scaled_bitmap = NULL;
> + glyph->scale = 0;
>
> /* Don't try to read empty bitmaps (e.g., space characters). */
> if (len != 0)
> @@ -787,6 +818,7 @@ grub_font_get_glyph_internal (grub_font_t font,
> grub_uint32_t code)
> if (grub_file_read (font->file, glyph->bitmap, len) != len)
> {
> remove_font (font);
> + grub_free (glyph->scaled_bitmap);
> grub_free (glyph);
> return 0;
> }
> @@ -1054,6 +1086,8 @@ grub_font_dup_glyph (struct grub_font_glyph *glyph)
> return NULL;
> grub_memcpy (ret, glyph, sizeof (*ret)
> + (glyph->width * glyph->height + 7) / 8);
> + ret->scaled_bitmap = NULL;
> + ret->scale = 0;
> return ret;
> }
> #endif
> @@ -1524,11 +1558,22 @@ grub_font_construct_glyph (grub_font_t hinted_font,
>
> if (max_glyph_size < sizeof (*glyph) + (bounds.width * bounds.height +
> GRUB_CHAR_BIT - 1) / GRUB_CHAR_BIT)
> {
> - grub_free (glyph);
> + if (glyph)
> + {
> + grub_free (glyph->scaled_bitmap);
> + grub_free (glyph);
> + }
> max_glyph_size = (sizeof (*glyph) + (bounds.width * bounds.height +
> GRUB_CHAR_BIT - 1) / GRUB_CHAR_BIT) * 2;
> if (max_glyph_size < 8)
> max_glyph_size = 8;
> glyph = grub_malloc (max_glyph_size);
> + if (glyph)
> + {
> + glyph->scaled_bitmap = NULL;
> + glyph->scale = 0;
> + }
> + else
> + max_glyph_size = 0;
> }
> if (!glyph)
> {
> @@ -1536,6 +1581,7 @@ grub_font_construct_glyph (grub_font_t hinted_font,
> return main_glyph;
> }
>
> + grub_free (glyph->scaled_bitmap);
> grub_memset (glyph, 0, sizeof (*glyph)
> + (bounds.width * bounds.height
> + GRUB_CHAR_BIT - 1) / GRUB_CHAR_BIT);
> @@ -1545,6 +1591,8 @@ grub_font_construct_glyph (grub_font_t hinted_font,
> glyph->height = bounds.height;
> glyph->offset_x = bounds.x;
> glyph->offset_y = bounds.y;
> + glyph->scaled_bitmap = NULL;
> + glyph->scale = 0;
>
> if (glyph_id->attributes & GRUB_UNICODE_GLYPH_ATTRIBUTE_MIRROR)
> grub_font_blit_glyph_mirror (glyph, main_glyph,
> @@ -1563,12 +1611,50 @@ grub_font_construct_glyph (grub_font_t hinted_font,
> return glyph;
> }
>
> +/* Try to scale the glyph bitmap. If failed, glyph will not be touched.
> + If succeeded, scaled bitmap will be stored at glyph->scaled_bitmap , and
> + the effective scale factor will be stored at glyph->scale . */
Incorrectly formatted comment. Please fix this. Here you can find more
details about comments formatting [1].
> +static void
> +try_scale_glyph (struct grub_font_glyph * glyph, int scale)
> +{
> + int i, j;
> + grub_uint8_t *bitmap;
> +
> + if (glyph->scale == scale)
> + return;
> +
> + if (scale > GRUB_FONT_MAX_SCALE
> + || glyph->height * scale > GRUB_FONT_MAX_DIMENSION
> + || glyph->width * scale > GRUB_FONT_MAX_DIMENSION)
> + return;
> +
> + bitmap = grub_zalloc (((glyph->width * scale) *
> + (glyph->height * scale) + 7) / 8);
grub_calloc()? And I think you should use grub_mul() and grub_add()
macros here and below...
> + if (!bitmap)
if (bitmap = NULL)
Please use NULL explicitly in such comparisons.
> + return;
> +
> + /* FIXME: suboptimal. */
> + for (i = 0; i < glyph->height * scale; i++)
> + for (j = 0; j < glyph->width * scale; j++)
> + {
> + int n = (i / scale) * glyph->width + (j / scale);
> + int m = i * (glyph->width * scale) + j;
> + int pixel = (glyph->bitmap[n / 8] >> (7 - n % 8)) & 1;
> + bitmap[m / 8] |= pixel << (7 - m % 8);
> + }
> +
> + grub_free (glyph->scaled_bitmap);
> + glyph->scaled_bitmap = bitmap;
> + glyph->scale = scale;
> +}
> +
> /* Draw the specified glyph at (x, y). The y coordinate designates the
> baseline of the character, while the x coordinate designates the left
> side location of the character. */
> grub_err_t
> grub_font_draw_glyph (struct grub_font_glyph * glyph,
> - grub_video_color_t color, int left_x, int baseline_y)
> + grub_video_color_t color, int left_x, int baseline_y,
> + int scale)
> {
> struct grub_video_bitmap glyph_bitmap;
>
> @@ -1601,11 +1687,29 @@ grub_font_draw_glyph (struct grub_font_glyph * glyph,
> &glyph_bitmap.mode_info.fg_alpha);
> glyph_bitmap.data = glyph->bitmap;
>
> - int bitmap_left = left_x + glyph->offset_x;
> - int bitmap_bottom = baseline_y - glyph->offset_y;
> - int bitmap_top = bitmap_bottom - glyph->height;
> + if (scale > 1)
> + {
> + try_scale_glyph (glyph, scale);
> + if (glyph->scale == scale)
> + {
> + glyph_bitmap.mode_info.width = glyph->width * scale;
> + glyph_bitmap.mode_info.height = glyph->height * scale;
> + glyph_bitmap.mode_info.pitch = glyph->width * scale;
> + glyph_bitmap.data = glyph->scaled_bitmap;
> + }
> + else
> + {
> + /* Scaled bitmap not suitable, fallback to no-scale. */
> + scale = 1;
> + }
> + }
> +
> + int bitmap_left = left_x + glyph->offset_x * scale;
> + int bitmap_bottom = baseline_y - glyph->offset_y * scale;
> + int bitmap_top = bitmap_bottom - glyph->height * scale;
You do a lot of scaling math here and there. It would be probably better
if you use safe math macros wrapped in some scaling functions/macros.
> return grub_video_blit_bitmap (&glyph_bitmap, GRUB_VIDEO_BLIT_BLEND,
> bitmap_left, bitmap_top,
> - 0, 0, glyph->width, glyph->height);
> + 0, 0,
> + glyph->width * scale, glyph->height * scale);
> }
> diff --git a/grub-core/gfxmenu/font.c b/grub-core/gfxmenu/font.c
> index 756c24f20..ed59ca954 100644
> --- a/grub-core/gfxmenu/font.c
> +++ b/grub-core/gfxmenu/font.c
> @@ -67,7 +67,7 @@ grub_font_draw_string (const char *str, grub_font_t font,
> err = grub_errno;
> goto out;
> }
> - err = grub_font_draw_glyph (glyph, color, x, baseline_y);
> + err = grub_font_draw_glyph (glyph, color, x, baseline_y, 1);
> if (err)
> goto out;
> x += glyph->device_width;
> diff --git a/grub-core/term/gfxterm.c b/grub-core/term/gfxterm.c
> index 3c468f459..4512dee6f 100644
> --- a/grub-core/term/gfxterm.c
> +++ b/grub-core/term/gfxterm.c
> @@ -656,7 +656,7 @@ paint_char (unsigned cx, unsigned cy)
> /* Render glyph to text layer. */
> grub_video_set_active_render_target (text_layer);
> grub_video_fill_rect (bgcolor, x, y, width, height);
> - grub_font_draw_glyph (glyph, color, x, y + ascent);
> + grub_font_draw_glyph (glyph, color, x, y + ascent, 1);
> grub_video_set_active_render_target (render_target);
>
> /* Mark character to be drawn. */
> diff --git a/include/grub/font.h b/include/grub/font.h
> index 708fa42ac..8fc646713 100644
> --- a/include/grub/font.h
> +++ b/include/grub/font.h
> @@ -24,6 +24,13 @@
> #include <grub/file.h>
> #include <grub/unicode.h>
>
> +/* Limit font dimensions to avoid interger overflow. Because some font
> metrics
> + may be derived from this limit, we must reserve some space for
> arithmetics,
> + so this value can't be too high. When scaling fonts, this limit should
> also
> + applies to the font after scaling. */
Please fix this comment formatting.
> +#define GRUB_FONT_MAX_DIMENSION 500
> +#define GRUB_FONT_MAX_SCALE 10
> +
> /* Forward declaration of opaque structure grub_font.
> Users only pass struct grub_font pointers to the font module functions,
> and do not have knowledge of the structure contents. */
Ditto and below please...
> @@ -79,6 +86,12 @@ struct grub_font_glyph
> /* Number of pixels to advance to start the next character. */
> grub_uint16_t device_width;
>
> + /* Pointer to cached scaled bitmap. Allocated during a scaling drawing.
> */
> + grub_uint8_t *scaled_bitmap;
> +
> + /* The scale factor for cached scaled bitmap. */
> + grub_uint16_t scale;
> +
> /* Row-major order, packed bits (no padding; rows can break within a byte).
> The length of the array is (width * height + 7) / 8. Within a
> byte, the most significant bit is the first (leftmost/uppermost) pixel.
> @@ -141,7 +154,8 @@ struct grub_font_glyph *EXPORT_FUNC
> (grub_font_get_glyph_with_fallback) (grub_fo
>
> grub_err_t EXPORT_FUNC (grub_font_draw_glyph) (struct grub_font_glyph *glyph,
> grub_video_color_t color,
> - int left_x, int baseline_y);
> + int left_x, int baseline_y,
> + int scale);
>
> int
> EXPORT_FUNC (grub_font_get_constructed_device_width) (grub_font_t
> hinted_font,
Daniel
[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
