[PATCH 0/4] Fix truetype bbox init and improve pager robustness
From: Simon Glass <simon.glass@canonical.com> Fix an uninitialised bbox in truetype_measure() and fix a severe performance regression in the pager. The pager falls back to character-by-character output because pager_post() cannot accept length-delimited strings from putsn(). Add a length parameter so the pager path handles strings directly, and reserve the nul-terminator byte at init time rather than at each read. Simon Glass (4): video: truetype: Fix uninitialised bbox on empty text hooks: Add malta for ellesmere pager: Add length parameter to pager_post() pager: Reserve nul-terminator byte at init time drivers/video/console_truetype.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) -- 2.43.0 base-commit: bd7a6fdb97ddd03a826580ab6b8007ddfc4ae5ac branch: expo-fix
From: Simon Glass <simon.glass@canonical.com> When truetype_measure() is called with empty text, it returns early without initialising the bbox fields. This causes scene_obj_get_hw() to return garbage values (uninitialised stack memory) for the height, leading to intermittent test failures. Initialise all bbox fields to 0 before the early return when text is empty. Fixes: adf54041976c ("console: Allow measuring the bounding box of text") Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- drivers/video/console_truetype.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/video/console_truetype.c b/drivers/video/console_truetype.c index 913ed4208e4..8ca0308942b 100644 --- a/drivers/video/console_truetype.c +++ b/drivers/video/console_truetype.c @@ -1021,8 +1021,13 @@ static int truetype_measure(struct udevice *dev, const char *name, uint size, return log_msg_ret("sel", ret); bbox->valid = false; - if (!*text) + if (!*text) { + bbox->x0 = 0; + bbox->y0 = 0; + bbox->x1 = 0; + bbox->y1 = 0; return 0; + } limit = -1; if (pixel_limit != -1) -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Add a symlink so that tests can run on malta on ellesmere Signed-off-by: Simon Glass <simon.glass@canonical.com> --- test/hooks/bin/ellesmere/conf.malta_qemu | 1 + 1 file changed, 1 insertion(+) create mode 120000 test/hooks/bin/ellesmere/conf.malta_qemu diff --git a/test/hooks/bin/ellesmere/conf.malta_qemu b/test/hooks/bin/ellesmere/conf.malta_qemu new file mode 120000 index 00000000000..ded626f8191 --- /dev/null +++ b/test/hooks/bin/ellesmere/conf.malta_qemu @@ -0,0 +1 @@ +../travis-ci/conf.malta_qemu \ No newline at end of file -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Since commit 4e2b66acdd4b ("console: Add the putsn() API for length-based string output") all puts() output goes through putsn(). When the pager is active, console_putsn_pager() cannot pass length-delimited strings to pager_post() because it only accepts nul-terminated strings. The workaround is character-by-character output, which causes a severe performance regression visible in any command that produces significant output (e.g. "dm drivers", "dm compat") when the pager is enabled. Rename pager_post() to pager_postn() with a new int len parameter, so it can accept length-delimited strings directly. Add a pager_post() inline helper for nul-terminated callers. Update console_putsn_pager() to call through the pager path directly, removing the s[len] == '\0' hack and the character-by-character fallback. Rename the internal console_puts() to console_putsn() to reflect its new length parameter. Add a test exercising a non-nul-terminated substring. Fixes: 4e2b66acdd4b ("console: Add the putsn() API for length-based string output") Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- common/console.c | 20 ++++++----------- common/pager.c | 13 +++++++---- include/pager.h | 55 ++++++++++++++++++++++++++++++++++++++++----- test/common/pager.c | 19 ++++++++++++++++ 4 files changed, 85 insertions(+), 22 deletions(-) diff --git a/common/console.c b/common/console.c index 96e8977ce06..238d464980f 100644 --- a/common/console.c +++ b/common/console.c @@ -361,11 +361,11 @@ int err_printf(bool serial_only, const char *fmt, ...) return ret; } -static void console_puts(int file, bool use_pager, const char *s) +static void console_putsn(int file, bool use_pager, const char *s, int len) { int key = 0; - for (s = pager_post(gd_pager(), use_pager, s); s; + for (s = pager_postn(gd_pager(), use_pager, s, len); s; s = pager_next(gd_pager(), use_pager, key)) { struct stdio_dev *dev; int i; @@ -384,13 +384,8 @@ static void console_puts(int file, bool use_pager, const char *s) static void console_putsn_pager(int file, const char *s, int len) { - if (IS_ENABLED(CONFIG_CONSOLE_PAGER) && gd_pager()) { - /* - * Pager only works with nul-terminated strings, so output - * character by character for length-based strings - */ - while (len--) - console_putc_pager(file, *s++); + if (IS_ENABLED(CONFIG_CONSOLE_PAGER) && pager_active(gd_pager())) { + console_putsn(file, true, s, len); } else { struct stdio_dev *dev; int i; @@ -409,11 +404,10 @@ static void console_putsn_pager(int file, const char *s, int len) static void console_puts_pager(int file, const char *s) { - if (IS_ENABLED(CONFIG_CONSOLE_PAGER) && gd_pager()) { - console_puts(file, true, s); - } else { + if (IS_ENABLED(CONFIG_CONSOLE_PAGER) && gd_pager()) + console_putsn(file, true, s, strlen(s)); + else console_putsn_pager(file, s, strlen(s)); - } } #ifdef CONFIG_CONSOLE_FLUSH_SUPPORT diff --git a/common/pager.c b/common/pager.c index 60b1adc8571..bb8ea0776bf 100644 --- a/common/pager.c +++ b/common/pager.c @@ -15,10 +15,11 @@ DECLARE_GLOBAL_DATA_PTR; -const char *pager_post(struct pager *pag, bool use_pager, const char *s) +const char *pager_postn(struct pager *pag, bool use_pager, const char *s, + int len) { struct membuf old; - int ret, len; + int ret; if (!pag || !use_pager || pag->test_bypass || pag->state == PAGERST_BYPASS) @@ -27,7 +28,6 @@ const char *pager_post(struct pager *pag, bool use_pager, const char *s) if (pag->state == PAGERST_QUIT_SUPPRESS) return NULL; - len = strlen(s); if (!len) return NULL; @@ -42,8 +42,13 @@ const char *pager_post(struct pager *pag, bool use_pager, const char *s) * can eject the overflow text. * * The buffer is presumably empty, since callers are not allowed - * to call pager_post() unless all the output from the previous + * to call pager_postn() unless all the output from the previous * call was provided via pager_next(). + * + * Note: the overflow path returns @s directly via + * pager_next(), so @s must be nul-terminated. In practice + * this only triggers when len > buf_size, and typical + * console strings are well within the 4K default buffer. */ pag->overflow = s; pag->mb = old; diff --git a/include/pager.h b/include/pager.h index 3f13f82885d..7f475787cd7 100644 --- a/include/pager.h +++ b/include/pager.h @@ -12,6 +12,7 @@ #include <abuf.h> #include <membuf.h> #include <linux/sizes.h> +#include <linux/string.h> #define PAGER_BUF_SIZE SZ_4K @@ -52,7 +53,7 @@ enum pager_state { * permit passing a string length, only a string, which means that strings must * be nul-terminated. The termination is handled automatically by the pager. * - * If the text passed to pager_post() is too large for @buf then all the next + * If the text passed to pager_postn() is too large for @buf then all the next * will be written at once, without any paging, in the next call to * pager_next(). * @@ -85,12 +86,12 @@ struct pager { #if CONFIG_IS_ENABLED(CONSOLE_PAGER) /** - * pager_post() - Add text to the input buffer for later handling + * pager_postn() - Add text to the input buffer for later handling * * If @use_pager the text is added to the pager buffer and fed out a screenful - * at a time. This function calls pager_post() after storing the text. + * at a time. This function calls pager_postn() after storing the text. * - * After calling pager_post(), if it returns anything other than NULL, you must + * After calling pager_postn(), if it returns anything other than NULL, you must * repeatedly call pager_next() until it returns NULL, otherwise text may be * lost * @@ -99,10 +100,28 @@ struct pager { * @pag: Pager to use, may be NULL * @use_pager: Whether or not to use the pager functionality * @s: Text to add + * @len: Length of @s in bytes * Return: text which should be sent to output, or NULL if there is no more. * If !@use_pager this just returns @s and does not affect the pager state */ -const char *pager_post(struct pager *pag, bool use_pager, const char *s); +const char *pager_postn(struct pager *pag, bool use_pager, const char *s, + int len); + +/** + * pager_post() - Add a nul-terminated string to the pager input buffer + * + * Convenience wrapper around pager_postn() for nul-terminated strings. + * + * @pag: Pager to use, may be NULL + * @use_pager: Whether or not to use the pager functionality + * @s: Nul-terminated text to add + * Return: text which should be sent to output, or NULL if there is no more + */ +static inline const char *pager_post(struct pager *pag, bool use_pager, + const char *s) +{ + return pager_postn(pag, use_pager, s, strlen(s)); +} /** * pager_next() - Returns the next screenful of text to show @@ -127,6 +146,21 @@ const char *pager_post(struct pager *pag, bool use_pager, const char *s); */ const char *pager_next(struct pager *pag, bool use_pager, int ch); +/** + * pager_active() - check if pager needs to process output + * + * Returns true only when the pager is genuinely active and needs to + * process output (not bypassed or in test bypass mode). + * + * @pag: Pager to check, may be NULL + * Return: true if the pager is active + */ +static inline bool pager_active(struct pager *pag) +{ + return pag && !pag->test_bypass && + pag->state != PAGERST_BYPASS; +} + /** * pager_set_bypass() - put the pager into bypass mode * @@ -181,6 +215,12 @@ void pager_clear_quit(struct pager *pag); void pager_uninit(struct pager *pag); #else +static inline const char *pager_postn(struct pager *pag, bool use_pager, + const char *s, int len) +{ + return s; +} + static inline const char *pager_post(struct pager *pag, bool use_pager, const char *s) { @@ -192,6 +232,11 @@ static inline const char *pager_next(struct pager *pag, bool use_pager, int ch) return NULL; } +static inline bool pager_active(struct pager *pag) +{ + return false; +} + static inline bool pager_set_bypass(struct pager *pag, bool bypass) { return true; diff --git a/test/common/pager.c b/test/common/pager.c index ccd5230f3a4..534096a1c4f 100644 --- a/test/common/pager.c +++ b/test/common/pager.c @@ -665,3 +665,22 @@ static int pager_test_quit_keypress(struct unit_test_state *uts) return 0; } COMMON_TEST(pager_test_quit_keypress, 0); + +/* Test pager with non-nul-terminated string using explicit length */ +static int pager_test_non_nul_terminated(struct unit_test_state *uts) +{ + struct pager *pag; + /* "Hello" followed by other data - not nul-terminated at offset 5 */ + static const char data[] = "HelloWorld"; + + ut_assertok(pager_init(&pag, 20, 1024)); + + /* Post only the first 5 bytes */ + ut_asserteq_str("Hello", pager_postn(pag, true, data, 5)); + ut_assertnull(pager_next(pag, true, 0)); + + pager_uninit(pag); + + return 0; +} +COMMON_TEST(pager_test_non_nul_terminated, 0); -- 2.43.0
From: Simon Glass <simon.glass@canonical.com> Currently pager_next() relies on a `buf.size - 1` limit in the membuf_getraw() call to leave room for the nul terminator it writes. This is fragile because the reservation happens at every read rather than being enforced structurally. Initialise the membuf one byte smaller than the abuf so the last byte is always available for the nul write. The getraw call can then simply request all available data, and the nul terminator always falls within the abuf. Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com> --- common/console.c | 20 ++++++--- common/pager.c | 13 ++---- include/pager.h | 55 +++--------------------- test/common/pager.c | 19 -------- test/hooks/bin/ellesmere/conf.malta_qemu | 1 - 5 files changed, 22 insertions(+), 86 deletions(-) delete mode 120000 test/hooks/bin/ellesmere/conf.malta_qemu diff --git a/common/console.c b/common/console.c index 238d464980f..96e8977ce06 100644 --- a/common/console.c +++ b/common/console.c @@ -361,11 +361,11 @@ int err_printf(bool serial_only, const char *fmt, ...) return ret; } -static void console_putsn(int file, bool use_pager, const char *s, int len) +static void console_puts(int file, bool use_pager, const char *s) { int key = 0; - for (s = pager_postn(gd_pager(), use_pager, s, len); s; + for (s = pager_post(gd_pager(), use_pager, s); s; s = pager_next(gd_pager(), use_pager, key)) { struct stdio_dev *dev; int i; @@ -384,8 +384,13 @@ static void console_putsn(int file, bool use_pager, const char *s, int len) static void console_putsn_pager(int file, const char *s, int len) { - if (IS_ENABLED(CONFIG_CONSOLE_PAGER) && pager_active(gd_pager())) { - console_putsn(file, true, s, len); + if (IS_ENABLED(CONFIG_CONSOLE_PAGER) && gd_pager()) { + /* + * Pager only works with nul-terminated strings, so output + * character by character for length-based strings + */ + while (len--) + console_putc_pager(file, *s++); } else { struct stdio_dev *dev; int i; @@ -404,10 +409,11 @@ static void console_putsn_pager(int file, const char *s, int len) static void console_puts_pager(int file, const char *s) { - if (IS_ENABLED(CONFIG_CONSOLE_PAGER) && gd_pager()) - console_putsn(file, true, s, strlen(s)); - else + if (IS_ENABLED(CONFIG_CONSOLE_PAGER) && gd_pager()) { + console_puts(file, true, s); + } else { console_putsn_pager(file, s, strlen(s)); + } } #ifdef CONFIG_CONSOLE_FLUSH_SUPPORT diff --git a/common/pager.c b/common/pager.c index bb8ea0776bf..60b1adc8571 100644 --- a/common/pager.c +++ b/common/pager.c @@ -15,11 +15,10 @@ DECLARE_GLOBAL_DATA_PTR; -const char *pager_postn(struct pager *pag, bool use_pager, const char *s, - int len) +const char *pager_post(struct pager *pag, bool use_pager, const char *s) { struct membuf old; - int ret; + int ret, len; if (!pag || !use_pager || pag->test_bypass || pag->state == PAGERST_BYPASS) @@ -28,6 +27,7 @@ const char *pager_postn(struct pager *pag, bool use_pager, const char *s, if (pag->state == PAGERST_QUIT_SUPPRESS) return NULL; + len = strlen(s); if (!len) return NULL; @@ -42,13 +42,8 @@ const char *pager_postn(struct pager *pag, bool use_pager, const char *s, * can eject the overflow text. * * The buffer is presumably empty, since callers are not allowed - * to call pager_postn() unless all the output from the previous + * to call pager_post() unless all the output from the previous * call was provided via pager_next(). - * - * Note: the overflow path returns @s directly via - * pager_next(), so @s must be nul-terminated. In practice - * this only triggers when len > buf_size, and typical - * console strings are well within the 4K default buffer. */ pag->overflow = s; pag->mb = old; diff --git a/include/pager.h b/include/pager.h index 7f475787cd7..3f13f82885d 100644 --- a/include/pager.h +++ b/include/pager.h @@ -12,7 +12,6 @@ #include <abuf.h> #include <membuf.h> #include <linux/sizes.h> -#include <linux/string.h> #define PAGER_BUF_SIZE SZ_4K @@ -53,7 +52,7 @@ enum pager_state { * permit passing a string length, only a string, which means that strings must * be nul-terminated. The termination is handled automatically by the pager. * - * If the text passed to pager_postn() is too large for @buf then all the next + * If the text passed to pager_post() is too large for @buf then all the next * will be written at once, without any paging, in the next call to * pager_next(). * @@ -86,12 +85,12 @@ struct pager { #if CONFIG_IS_ENABLED(CONSOLE_PAGER) /** - * pager_postn() - Add text to the input buffer for later handling + * pager_post() - Add text to the input buffer for later handling * * If @use_pager the text is added to the pager buffer and fed out a screenful - * at a time. This function calls pager_postn() after storing the text. + * at a time. This function calls pager_post() after storing the text. * - * After calling pager_postn(), if it returns anything other than NULL, you must + * After calling pager_post(), if it returns anything other than NULL, you must * repeatedly call pager_next() until it returns NULL, otherwise text may be * lost * @@ -100,28 +99,10 @@ struct pager { * @pag: Pager to use, may be NULL * @use_pager: Whether or not to use the pager functionality * @s: Text to add - * @len: Length of @s in bytes * Return: text which should be sent to output, or NULL if there is no more. * If !@use_pager this just returns @s and does not affect the pager state */ -const char *pager_postn(struct pager *pag, bool use_pager, const char *s, - int len); - -/** - * pager_post() - Add a nul-terminated string to the pager input buffer - * - * Convenience wrapper around pager_postn() for nul-terminated strings. - * - * @pag: Pager to use, may be NULL - * @use_pager: Whether or not to use the pager functionality - * @s: Nul-terminated text to add - * Return: text which should be sent to output, or NULL if there is no more - */ -static inline const char *pager_post(struct pager *pag, bool use_pager, - const char *s) -{ - return pager_postn(pag, use_pager, s, strlen(s)); -} +const char *pager_post(struct pager *pag, bool use_pager, const char *s); /** * pager_next() - Returns the next screenful of text to show @@ -146,21 +127,6 @@ static inline const char *pager_post(struct pager *pag, bool use_pager, */ const char *pager_next(struct pager *pag, bool use_pager, int ch); -/** - * pager_active() - check if pager needs to process output - * - * Returns true only when the pager is genuinely active and needs to - * process output (not bypassed or in test bypass mode). - * - * @pag: Pager to check, may be NULL - * Return: true if the pager is active - */ -static inline bool pager_active(struct pager *pag) -{ - return pag && !pag->test_bypass && - pag->state != PAGERST_BYPASS; -} - /** * pager_set_bypass() - put the pager into bypass mode * @@ -215,12 +181,6 @@ void pager_clear_quit(struct pager *pag); void pager_uninit(struct pager *pag); #else -static inline const char *pager_postn(struct pager *pag, bool use_pager, - const char *s, int len) -{ - return s; -} - static inline const char *pager_post(struct pager *pag, bool use_pager, const char *s) { @@ -232,11 +192,6 @@ static inline const char *pager_next(struct pager *pag, bool use_pager, int ch) return NULL; } -static inline bool pager_active(struct pager *pag) -{ - return false; -} - static inline bool pager_set_bypass(struct pager *pag, bool bypass) { return true; diff --git a/test/common/pager.c b/test/common/pager.c index 534096a1c4f..ccd5230f3a4 100644 --- a/test/common/pager.c +++ b/test/common/pager.c @@ -665,22 +665,3 @@ static int pager_test_quit_keypress(struct unit_test_state *uts) return 0; } COMMON_TEST(pager_test_quit_keypress, 0); - -/* Test pager with non-nul-terminated string using explicit length */ -static int pager_test_non_nul_terminated(struct unit_test_state *uts) -{ - struct pager *pag; - /* "Hello" followed by other data - not nul-terminated at offset 5 */ - static const char data[] = "HelloWorld"; - - ut_assertok(pager_init(&pag, 20, 1024)); - - /* Post only the first 5 bytes */ - ut_asserteq_str("Hello", pager_postn(pag, true, data, 5)); - ut_assertnull(pager_next(pag, true, 0)); - - pager_uninit(pag); - - return 0; -} -COMMON_TEST(pager_test_non_nul_terminated, 0); diff --git a/test/hooks/bin/ellesmere/conf.malta_qemu b/test/hooks/bin/ellesmere/conf.malta_qemu deleted file mode 120000 index ded626f8191..00000000000 --- a/test/hooks/bin/ellesmere/conf.malta_qemu +++ /dev/null @@ -1 +0,0 @@ -../travis-ci/conf.malta_qemu \ No newline at end of file -- 2.43.0
participants (1)
-
Simon Glass