bug#40897: Negation of pixel-width :width expressions does not work

bug#40897: Negation of pixel-width :width expressions does not work

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]

In looking over the xdisp.c code, I noticed that
calc_pixel_width_or_height has at least three problems:

1. negation of width specifiers doesn't work
2. a circular list passed to (+) will hang Emacs
3. a circular list passed directly will crash Emacs

(1) is a significant bug and should be fixed. (2) is easy to fix, and
we should probably do so on the master branch. (3) is difficult to
fix, but I'd like to fix all these, and undiscovered similar issues,
by converting calc_pixel_width_or_height to using the Lisp
interpreter.

The underlying problem is we're writing yet another mini-interpreter
for a limited Lisp-like language, in C. That means no quitting, no
protection against stack overflows, and getting tricky semantics (such
as the different behavior of (- A) and (- A B ...)) wrong.

My proposal is to move the somewhat complex logic of
calc_pixel_width_or_height to Lisp code, run when the display spec is
actually evaluated (i.e. when "(when ...)" display specs run their
Lisp code). This could be achieved by a method similar to the patch
for bug#40845 in
https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-04/msg01096.html.

This would also resolve bug#40856 by allowing more complicated space
calculations at a time when the font metrics are known.

I'm attaching a patch, against master, for (1) only. I haven't tried
to restore the previous "optimization" of avoiding (very cheap) calls
to EQ for all but the first argument.

Recipes:

(1):

(insert (propertize " " 'display '(space :width (+ 50 (- 50)))))

Expected result: a zero-width space
Actual result: a space of width 100

(2):
(let ((l (cons 0 nil)))
  (setcdr l l)
  (insert (propertize " " 'display `(space :width (+ . ,l)))))

Expected result: a quittable infinite loop
Actual result: a non-quittable infinite loop

(3):
(let ((l (cons 0 nil)))
  (setcdr l l)
  (insert (propertize " " 'display `(space :width ,l))))

Expected result: a Lisp stack overflow error
Actual result: a C stack overflow, Emacs crashes

[-- Attachment #2: 0001-Fix-pixel-width-evaluation-of-negated-specs.patch --]
[-- Type: text/x-patch, Size: 1344 bytes --]

From b7493ff934553a2d008cce4fb8844d25fd2df998 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Mon, 27 Apr 2020 08:41:52 +0000
Subject: [PATCH] Fix pixel-width evaluation of negated specs

* src/xdisp.c (calc_pixel_width_or_height): Fix evaluation of (- X)
SPEC.
---
 src/xdisp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 140d134572..1aba4c8901 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -27278,7 +27278,7 @@ calc_pixel_width_or_height (double *res, struct it *it, Lisp_Object prop,
 	     recursively calculated values.  */
 	  if (EQ (car, Qplus) || EQ (car, Qminus))
 	    {
-	      bool first = true;
+	      ptrdiff_t count = 0;
 	      double px;
 
 	      pixels = 0;
@@ -27287,13 +27287,13 @@ calc_pixel_width_or_height (double *res, struct it *it, Lisp_Object prop,
 		  if (!calc_pixel_width_or_height (&px, it, XCAR (cdr),
 						   font, width_p, align_to))
 		    return false;
-		  if (first)
-		    pixels = (EQ (car, Qplus) ? px : -px), first = false;
-		  else
+		  if (count++ == 0)
 		    pixels += px;
+		  else
+		    pixels += (EQ (car, Qplus) ? px : -px);
 		  cdr = XCDR (cdr);
 		}
-	      if (EQ (car, Qminus))
+	      if (EQ (car, Qminus) && count == 1)
 		pixels = -pixels;
 	      return OK_PIXELS (pixels);
 	    }
-- 
2.26.2

bug#40897: Negation of pixel-width :width expressions does not work

[Pip Cet]

On Mon, Apr 27, 2020 at 8:49 AM Pip Cet <pipcet@gmail.com> wrote:
> 3. a circular list passed directly will crash Emacs

I'm not saying this is a security issue, but you can create a
text/enriched file, in the obvious way, which will crash emacs -Q when
you open it. That's very inconvenient.

bug#40897: Negation of pixel-width :width expressions does not work

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Mon, 27 Apr 2020 08:47:50 +0000
> 
> In looking over the xdisp.c code, I noticed that
> calc_pixel_width_or_height has at least three problems:
> 
> 1. negation of width specifiers doesn't work

Thanks.  That should tell us how many people tried to use this feature
since it was introduced, 16 years ago ;-)

> 2. a circular list passed to (+) will hang Emacs
> 3. a circular list passed directly will crash Emacs
> 
> (1) is a significant bug and should be fixed. (2) is easy to fix, and
> we should probably do so on the master branch. (3) is difficult to
> fix, but I'd like to fix all these, and undiscovered similar issues,
> by converting calc_pixel_width_or_height to using the Lisp
> interpreter.

I don't think converting this to Lisp is a good idea, and I try to
explain below why.  It doesn't seem to be needed for fixing these
particular problems (see the proposed patch below), and even more so
given the evidence of this feature's popularity.

> The underlying problem is we're writing yet another mini-interpreter
> for a limited Lisp-like language, in C.

That is true, but there's a reason to this.

> That means no quitting, no protection against stack overflows

You cannot usefully quit or protect against stack overflow inside
redisplay anyway.  Quitting or signaling an error during redisplay
just gives you an endless redisplay loop, because signaling an error
immediately enters another redisplay cycle.  And even if you catch the
error and quietly return, in many/most cases you have another
redisplay either immediately or soon enough, and that presents a
locked-up or almost a locked-up Emacs ("almost" means that typing M-<
or M-> enough time might eventually get you out of the vicious circle,
if the problematic place is not at BOB/EOB).

The only reasonable way of avoiding these is to prevent the need to
error out.

You say (3) is difficult to fix, but the patch below does succeed in
preventing the stack overflow, so I don't think I understand why you
thought this to be difficult.  What am I missing?

> getting tricky semantics (such as the different behavior of (- A)
> and (- A B ...)) wrong.

AFAIU, the semantics is the same as in Lisp, isn't it?  The
implementation has a bug, but the semantics doesn't, I think.

> My proposal is to move the somewhat complex logic of
> calc_pixel_width_or_height to Lisp code, run when the display spec is
> actually evaluated (i.e. when "(when ...)" display specs run their
> Lisp code). This could be achieved by a method similar to the patch
> for bug#40845 in
> https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-04/msg01096.html.
> 
> This would also resolve bug#40856 by allowing more complicated space
> calculations at a time when the font metrics are known.

You have promoted similar views elsewhere, on at least 2 other
occasions, so let me now try to explain why I very much object to
moving display functionality to Lisp, as a general tendency (as
opposed to some selected circumstances where this could be justified).

My reasons are as follows:

  . Lisp slows down redisplay.  First it does that because it's Lisp,
    and second because it causes GC.  We already have enough
    complaints against redisplay slowness (even on today's fast
    desktop machines!), so we should be very careful about this.

  . Handling Lisp errors inside redisplay is tricky, for the reasons
    explained above.  It is even more tricky when Lisp computes
    something that the rest of redisplay needs for its operation (as
    opposed to calling various hooks, such as window-scroll-functions,
    which are basically a one-way street -- if the hook errors out, it
    doesn't disrupt the display itself).

  . Last, but not least -- and people tend to forget about this
    important factor -- using Lisp to calculate display elements makes
    it nigh impossible to know when certain redisplay optimizations
    can be validly applied and when not.  That's because A Lisp
    program can potentially do anything and depend on any numbers of
    global and local state variables, and users will expect the
    display to change when these variables change their values.
    Redisplay optimizations are based on tracking some key state
    variables to deduce when a more thorough redisplay is needed and
    when it is safe to use shortcuts.  This is already a hard task,
    especially since we consistently made Emacs redisplay more and
    more lazily starting from Emacs 25 -- we still get bug reports
    about some situations where some state change fails to trigger
    redisplay, even though the number of possible variables to track
    is not very large.  Using Lisp much more than we do now will make
    tracking the relevant variables impossible.  The result will be
    only one: we will have to disable more and more optimizations and
    shortcuts to keep what's on the glass accurate.  And Emacs's
    redisplay can be *really* slow without the optimizations,
    especially in simple cases, like when you just move the cursor, or
    delete or insert a character.

So my suggestion is to use Lisp as part of redisplay only where no
other solution is reasonable or practical, and when the feature we
want is really important to have.

Once again, there could be certain situations where calling Lisp from
the display code might be the only practical solution for some serious
and important problem.  The above just tries to explain why I don't
think that idea is good _in_general_, and why we should try to avoid
it if at all possible.

I hope I made my position on this clear enough.

Here's the patch I came up with to handle the problems you reported.
WDYT?

diff --git a/src/xdisp.c b/src/xdisp.c
index 140d134572..bc27fb15e0 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -27284,11 +27284,16 @@ calc_pixel_width_or_height (double *res, struct it *it, Lisp_Object prop,
 	      pixels = 0;
 	      while (CONSP (cdr))
 		{
+		  if (EQ (cdr, XCDR (cdr)))
+		    return false;
 		  if (!calc_pixel_width_or_height (&px, it, XCAR (cdr),
 						   font, width_p, align_to))
 		    return false;
 		  if (first)
-		    pixels = (EQ (car, Qplus) ? px : -px), first = false;
+		    pixels =
+		      (EQ (car, Qminus) && CONSP (XCDR (cdr))
+		       ? -px
+		       : px), first = false;
 		  else
 		    pixels += px;
 		  cdr = XCDR (cdr);
@@ -27305,13 +27310,15 @@ calc_pixel_width_or_height (double *res, struct it *it, Lisp_Object prop,
 
       /* '(NUM)': absolute number of pixels.  */
       if (NUMBERP (car))
-{
+	{
 	  double fact;
 	  int offset =
 	    width_p && align_to && *align_to < 0 ? it->lnum_pixel_width : 0;
 	  pixels = XFLOATINT (car);
 	  if (NILP (cdr))
 	    return OK_PIXELS (pixels + offset);
+	  if (EQ (cdr, XCDR (cdr)))
+	    return false;
 	  if (calc_pixel_width_or_height (&fact, it, cdr,
 					  font, width_p, align_to))
 	    return OK_PIXELS (pixels * fact + offset);

bug#40897: Negation of pixel-width :width expressions does not work

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Mon, 27 Apr 2020 12:18:41 +0000
> 
> I'm not saying this is a security issue, but you can create a
> text/enriched file, in the obvious way, which will crash emacs -Q when
> you open it. That's very inconvenient.

Sure, crashes caused by Lisp should be fixed.

bug#40897: Negation of pixel-width :width expressions does not work

[Pip Cet]

On Mon, Apr 27, 2020 at 2:32 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Mon, 27 Apr 2020 08:47:50 +0000
> You say (3) is difficult to fix, but the patch below does succeed in
> preventing the stack overflow, so I don't think I understand why you
> thought this to be difficult.  What am I missing?

Circular lists can have a cycle of length greater than one. However, a
simple limitation of how deeply we recurse into the display spec would
work, and potentially be more robust. Would you agree with that?

Thank you for your other explanations. I've taken the opportunity to
have a look at the redisplay code. I'll confess it's a bit
overwhelming in its complexity, and my understanding so far is
tentative, but it seems to me like it "essentially" goes from buffer
text to the glyph matrix in one shot: there's no intermediate state
that represents what CSS would call the "resolved style" of the
document.

Trying to write this email, I'm realizing I need to think about this
more: I still think the current state of things, where we call Lisp as
part of redisplay but usually don't rely on it, is much better than to
exclude Lisp from the process entirely.

bug#40897: Negation of pixel-width :width expressions does not work

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 1 May 2020 09:41:51 +0000
> Cc: 40897@debbugs.gnu.org
> 
> On Mon, Apr 27, 2020 at 2:32 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > > From: Pip Cet <pipcet@gmail.com>
> > > Date: Mon, 27 Apr 2020 08:47:50 +0000
> > You say (3) is difficult to fix, but the patch below does succeed in
> > preventing the stack overflow, so I don't think I understand why you
> > thought this to be difficult.  What am I missing?
> 
> Circular lists can have a cycle of length greater than one. However, a
> simple limitation of how deeply we recurse into the display spec would
> work, and potentially be more robust. Would you agree with that?

Yes, I agree.  But maybe we should do that in addition to the simple
check I suggested, as that is much faster.  Of course, I have no idea
of the relative frequency of each of these use cases, but a single EQ
test seems a small price to pay.

> Thank you for your other explanations. I've taken the opportunity to
> have a look at the redisplay code. I'll confess it's a bit
> overwhelming in its complexity, and my understanding so far is
> tentative

Feel free to ask questions, I'm very much interested in spreading the
knowledge about the display engine internals among as many regular
contributors as possible.

> but it seems to me like it "essentially" goes from buffer text to
> the glyph matrix in one shot: there's no intermediate state that
> represents what CSS would call the "resolved style" of the document.

I'm no CSS expert, but AFAIU we don't have a single equivalent of what
CSS can do.  We do have a large portion of that, though: the faces.
There, we do have the equivalent of the "resolved style", IIUC: the
"realized face", see xfaces.c.  As part of producing the glyphs in the
glyph matrices, the display engine does realize faces that were not
(yet) realized, for example, when it finds a character whose
appearance is affected by several sources of face information (text
properties, overlays, mouse-face, etc.).  But this is done on the fly,
without a separate pass: the pertinent faces are merged in the
appropriate order, then the merged face is realized and cached, and
the iteration through text continues using this cached face ID.  See
the function handle_stop in xdisp.c, which is the focal point for
handling anything "unusual" the display engine bumps into while
walking the text to display.

> Trying to write this email, I'm realizing I need to think about this
> more: I still think the current state of things, where we call Lisp as
> part of redisplay but usually don't rely on it, is much better than to
> exclude Lisp from the process entirely.

We don't exclude Lisp entirely.  You will see quite a few hooks we
call along the way, and also some non-hooks; a notable example of the
latter is fontification-functions.  What I'm saying is that we need to
keep this in check, for those use cases and situations which are
important and cannot be reasonably supported via variables or simple
forms.  We shouldn't add Lisp calls in redisplay "just because we
can."