From 3abf2b82602fd34e792b2e29b925a877c265a045 Mon Sep 17 00:00:00 2001
From: Qeole <qeole@outlook.com>
Date: Thu, 10 Feb 2022 20:21:47 +0000
Subject: [PATCH] Faces: Check that underline colour comes before
 base/attributes markers

Parsing a (non-valid) font with a comma in the name of the base colour
makes Kakoune crash. It is not a valid face, but Kakoune should just
return an error message instead.

Reproducer:

    :set-face global foo ,red@,blue

Note the comma "," after the "@". This is not a valid base name, and it
leads to a crash. Let's see what happens.

At the beginning of parse_face(), we have the following code:

    auto bg_it = find(facedesc, ',');
    auto underline_it = bg_it == facedesc.end() ? bg_it : std::find(bg_it+1, facedesc.end(), ',');
    auto attr_it = find(facedesc, '+');
    auto base_it = find(facedesc, '@');
    [...]
    auto colors_end = std::min(attr_it, base_it);

After this:

- bg_it points to ",red@,blue"
- bg_it != facedesc.end(), so we have underline_it pointing to the first
  comma after bg_it. This means that underline_it points to ",blue"
- base_it points to "@,blue"
- attr_it points to the end of facedesc (no "+" marker), so colors_end
  points to base_it, "@,blue"

Later in the code, just after parsing the foreground and background
colours, we have:

    if (underline_it != facedesc.end())
        face.underline = parse_color({underline_it+1, colors_end});

When passing {underline_it+1, colors_end} to parse_color(), we pass in
fact iterators pointing to {",blue", "@,blue"}. Because the second one
starts _before_ the first one in the string, this means that the
resulting string is considered to have a _negative_ length.
parse_color() passes the string to str_to_color(), who fails to turn up
the colour, and attempts to throw:

    throw runtime_error(format("unable to parse color: '{}'", color));

The variable "color" still has this negative length, and this goes all
the way down to an assert in src/units.hh where we expect that string to
be >= 0, and we crash on the assertion failure.

For similar reasons, we also get a crash if the comma comes after the
marker for the face attributes:

    :set-face global foo ,red+,a

To fix both cases, let's add a check to make sure that the underline_it,
marked with a comma, never gets detected as present and pointing after
colors_end, be it "@" or "+".
---
 src/face_registry.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/face_registry.cc b/src/face_registry.cc
index 84f6826b..e7803cac 100644
--- a/src/face_registry.cc
+++ b/src/face_registry.cc
@@ -25,6 +25,9 @@ static FaceRegistry::FaceSpec parse_face(StringView facedesc)
         throw runtime_error(invalid_face_error.str());
 
     auto colors_end = std::min(attr_it, base_it);
+    if (underline_it != facedesc.end()
+        and underline_it > colors_end)
+        throw runtime_error(invalid_face_error.str());
 
     auto parse_color = [](StringView spec) {
         return spec.empty() ? Color::Default : str_to_color(spec);