Re: Message's text-properties in *Messages*

[Eli Zaretskii]

> Date: Wed, 23 May 2018 21:07:37 +0300
> From: Eli Zaretskii <address@hidden>
> Cc: address@hidden
> 
> > > When Lisp code is called from redisplay, we always do that via
> > > safe_call.  Anything else is not safe, AFAIR.
> > 
> > I see.  This doesn't answer all the questions, tho:
> 
> I didn't mean to answer all of them yet, I will need some time to look
> at the code.

Sorry for the long delay in responding.

> I'm playing with the work-in-progress below for that, but I have
> a question about message_dolog.  Its comment says:
> 
>     /* Add a string M of length NBYTES to the message log, optionally
>        terminated with a newline when NLFLAG is true.  MULTIBYTE, if
>        true, means interpret the contents of M as multibyte.  This
>        function calls low-level routines in order to bypass text property
>        hooks, etc. which might not be safe to run.
>     
>        This may GC (insert may run before/after change hooks),
>        so the buffer M must NOT point to a Lisp string.  */

My first comment is that the first paragraph of this is very old, it
predates the complete rewrite of message_dolog during development of
Emacs 21.  So it is quite possible that parts of it could no longer be
relevant.  But see below.

> I find this somewhat confusing:
> - Not sure which text property hooks it's referring to.

My conclusion from looking at the code and its history is that the
comment refers to before-change-functions and maybe to the rest of
stuff done by signal_before_change.

> - It first says "... hooks, etc. which might not be safe to run" but later
>   "(insert may run before/after change hooks)".
>   Aren't before/after hooks just as dangerous as others?

This part tries to explain why the code avoids calling 'insert': it
does that precisely so as to avoid triggering before-change-functions,
the "text property hooks" being talked about here.  IOW, the part in
the parentheses wants to explain why message_dolog does NOT call
'insert', and instead uses the lower-level insert_1_both.

> - The code actually doesn't seem to run before/after change hooks.

It explicitly avoids running before-change-functions, by calling
functions that won't, yes.  But it does call after-change-functions,
if del_range_both is called by message_dolog.  Given my conclusion
(below), I think I understand why only the before-change-functions are
feared of.

> - But the code does call `messages-buffer-mode` (when (re)creating the
>   buffer), so it does run potentially arbitrary Lisp code.

The addition of messages-buffer-mode is relatively new, we have it
only since Sep 2013.  If that mode runs arbitrary Lisp, I think it
indeed could potentially cause trouble, and we are just lucky that we
didn't see it yet.

> - Why would arbitrary Lisp code be dangerous (I understand that message_dolog
>   can be called from within redisplay, but redisplay runs Elisp code
>   at several places, so "from redisplay" doesn't inherently imply you
>   can't run Elisp code).

As I wrote previously, redisplay is not the issue here, as
message_dolog doesn't call any redisplay entry points.

> To a large extent my question is whether those inhibit-<foo>-hooks would
> make it safe enough to replace message_dolog with message_dolog_lisp
> (aka rather reimplement message_dolog on top of message_dolog_lisp), or
> whether we really need to keep the "safer" message_dolog (in which case
> I'll have to work harder at sharing more code between the two).

The problem as I see it is actually explained in another place, in the
commentary to 'insert':

   DO NOT use this for the contents of a Lisp string or a Lisp buffer!
   prepare_to_modify_buffer could relocate the text.  */

This is the reason why message_dolog avoids calling 'insert': it wants
to support the use case where its first argument is a pointer to some
buffer's text or to contents of a Lisp string (as opposed to a C
string).  'insert' calls prepare_to_modify_buffer, which could cause
GC, which could relocate buffer or string text, and thus invalidate
the C pointer passed to message_dolog.  I see at least one call of
message_dolog, via strout, which passes a pointer to contents of a
Lisp string, so we still have such uses, and I'm not sure we want to
restrict message_dolog to not support such cases (I'm not sure we can
do that, even if we tried).

I think the same problem could happen as result of calling
messages-buffer-mode, if indeed it calls arbitrary Lisp and causes GC.

By contrast, after-change-functions are called when the text to add to
the log is no longer need, so we no longer care about GC and potential
relocations.

Coming back to your patch, my comments are:

  . the new function message_dolog_lisp is safe because it accepts a
    Lisp string, not a C 'char *' pointer
  . however, the invocation of messages-buffer-mode in message_dolog
    is dangerous, and I think we want to prevent any potential
    breakage due to GC there (but this is not caused by your current
    patch)
  . therefore, maybe we should move that call to messages-buffer-mode
    to the new function(?)

HTH