From 0716880f68960309371c9807bfda02ad0d92c41c Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 23 Jul 2019 18:58:45 +0100 Subject: [PATCH] stripctrl: clean up precarious handling of 'width'. In both stripctrl_locale_put_wc() and stripctrl_term_put_wc(), we get a character width from either wcwidth or term_char_width, which might be -1 for a control character. Coverity complained that that width value is later passed to to stripctrl_check_line_limit, which takes a size_t parameter, i.e. it expects the width to be non-negative. This is another bug that doesn't actually cause damage, but the reasons why are quite fiddly: - The only width<0 characters we let through are those that got through stripctrl_ctrlchar_ok. (At both call sites, if that function is unhappy then we either take an early return or else replace the character _and_ the value of 'width' with a substitution.) - stripctrl_ctrlchar_ok only lets through \n, or \r if permit_cr is set. - stripctrl_check_line_limit ignores its width parameter if line limiting is turned off, or if it gets \n. - So the only way this can cause a problem is if permit_cr is set so that \r can get through ctrlchar_ok, *and* line limiting is turned on so that we get far enough through check_line_limit to choke on its negative width. - But in fact all the call sites that ever call stripctrl_enable_line_limiting do it with a StripCtrlChars that they got from a seat, and all the nontrivial implementations of seat_stripctrl_new pass false for permit_cr. So the combination of circumstances in which -1 gets implicitly converted to size_t *and* stripctrl_check_line_limit actually pays attention to it can never arise. But it's clearly foolish to make the correctness of the code depend on a proof that long - now Coverity has pointed it out, I'm not happy with it either! Fixed by substituting 0 for the width in the questionable cases. --- stripctrl.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/stripctrl.c b/stripctrl.c index b5a8f05f..58289b10 100644 --- a/stripctrl.c +++ b/stripctrl.c @@ -162,6 +162,8 @@ static inline void stripctrl_locale_put_wc(StripCtrlCharsImpl *scc, wchar_t wc) int width = mk_wcwidth(wc); if ((iswprint(wc) && width >= 0) || stripctrl_ctrlchar_ok(scc, wc)) { /* Printable character, or one we're going to let through anyway. */ + if (width < 0) + width = 0; /* sanitise for stripctrl_check_line_limit */ } else if (scc->substitution) { wc = scc->substitution; width = mk_wcwidth(wc); @@ -196,6 +198,9 @@ static inline void stripctrl_term_put_wc( width = term_char_width(scc->term, wc); assert(width >= 0); } + } else { + if (width < 0) + width = 0; /* sanitise for stripctrl_check_line_limit */ } if (wc == '\012') {