bug#53808: 29.0.50; ansi colorization process could block indefinetly on stray ESC char

[Ioannis Kappas <ioannis.kappas@gmail.com>]

Hi Miha,

On Sun, Feb 6, 2022 at 8:30 PM <miha@kamnitnik.top> wrote:

> Thanks. I took the liberty of working on your patch, adding support for
> ansi-color-apply-on-region, ansi-color-filter-region,
> ansi-color-filter-apply. I also added some tests as you suggested and
> made a minor simplification.
>

thanks for looking into this! The patch looks good and reduces the
issue considerably, but I've noticed there is still some undesired
behaviour with non SGR CSI sequences. I was expecting the following
test to display the non SGR `\e[a' characters verbatim in the output
(this is in the context of the
test/lisp/ansi-color-tests.el:ansi-color-incomplete-sequences-test()),

(dolist (fun (list ansi-filt ansi-app))
        (with-temp-buffer
          (should (equal (funcall fun "\e[a") ""))
          (should (equal (funcall fun "\e[33m Z \e[0m")
                         (with-temp-buffer
                           (concat "\e[a" (funcall fun "\e[33m Z \e[0m")))))
          ))

but fails to do so with

Test ansi-color-incomplete-sequences-test condition:
    (ert-test-failed
     ((should
       (equal
        (funcall fun "\33[33m Z \33[0m")
        (with-temp-buffer ...)))
      :form
      (equal " Z " "\33[a Z ")
      :value nil :explanation
      (arrays-of-different-length 3 6 " Z " "\33[a Z " first-mismatch-at 0)))

i.e. the "\e[a" seq does not appear in the output. Even before that, I
was expecting  (equal (funcall fun "\e[a") "") to fail and (equal
(funcall fun "\e[a") "\e[a") to be true instead (as this can't be the
start of a valid SGR expression).

Is there a reason why the ansi-color library tries to match input
against the CSI superset sequence instead of the SGR subset? The
package appears to be dealing exclusively with the latter and using
CSI regexps seems like an unnecessary complication to me.

(Just for reference, I'm using the terminology found in the ANSI
escape code in wikipedia at
https://en.wikipedia.org/w/index.php?title=ANSI_escape_code&oldid=1070369816#Description)

The SGR set as I understand it is the char sequence starting with the
ESC control character followed by the [ character followed by zero or
more of [0-9]+; followed by [0-9]+ followed by m. For example, ESC[33m
or ESC[3;31m. This is what I tried to capture as a fragment with the
"\e\\(?:\\[\\|$\\)\\(?:(?:[0-9]+;?\\)*"  regexp in my original patch.

Another minor observation, perhaps the following concat could be moved
into defconst in the interest of performance (it appears twice in the
patch)?

     (let ((fragment ""))
       (push (substring string start
-                       (if (string-match "\033" string start)
+                       (if (string-match
+                            (concat "\\(?:"
ansi-color--control-seq-fragment-regexp "\\)\\'")
+                            string start)

Best Regards