From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Message's text-properties in *Messages* Date: Wed, 30 May 2018 20:19:50 +0300 Message-ID: <83tvqpgix5.fsf@gnu.org> References: <20180521171019.GA5750@ACM> <83a7sqcmnw.fsf@gnu.org> <83603eckk6.fsf@gnu.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1527700712 14109 195.159.176.226 (30 May 2018 17:18:32 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 30 May 2018 17:18:32 +0000 (UTC) Cc: emacs-devel@gnu.org To: monnier@iro.umontreal.ca Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed May 30 19:18:28 2018 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1fO4jz-0003Y9-Il for ged-emacs-devel@m.gmane.org; Wed, 30 May 2018 19:18:27 +0200 Original-Received: from localhost ([::1]:39881 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fO4m6-0000GF-FG for ged-emacs-devel@m.gmane.org; Wed, 30 May 2018 13:20:38 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:52150) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fO4lU-0000Ft-1z for emacs-devel@gnu.org; Wed, 30 May 2018 13:20:01 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fO4lO-0004Xg-KE for emacs-devel@gnu.org; Wed, 30 May 2018 13:19:59 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:59870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fO4lO-0004XV-Ft; Wed, 30 May 2018 13:19:54 -0400 Original-Received: from [176.228.60.248] (port=1303 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1fO4lN-00007c-Up; Wed, 30 May 2018 13:19:54 -0400 In-reply-to: <83603eckk6.fsf@gnu.org> (message from Eli Zaretskii on Wed, 23 May 2018 21:07:37 +0300) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:225827 Archived-At: > Date: Wed, 23 May 2018 21:07:37 +0300 > From: Eli Zaretskii > Cc: emacs-devel@gnu.org > > > > 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--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