bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Michael Heerdegen]

Hello,

emacs -Q:

(progn (readablep (current-window-configuration))
       (current-buffer))
==> #<buffer  prin1>

After evaluating such a `readablep' call, weird things can happen
because of this buffer being current.  I tried to actually display that
buffer and the window I got looked quite funny, one second later Emacs
had crashed.

TIA,

Michael.


In GNU Emacs 29.0.50 (build 31, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2022-07-26 built on drachen
Repository revision: c455023f93f5073e7032a2097821e23eafa6f1aa
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> (progn (readablep (current-window-configuration))
>        (current-buffer))
> ==> #<buffer  prin1>
>
> After evaluating such a `readablep' call, weird things can happen
> because of this buffer being current.  I tried to actually display that
> buffer and the window I got looked quite funny, one second later Emacs
> had crashed.

Oops.

The problem is trivial to fix in `unreadablep' (just smack a
`save-excursion' in there), but I think this should be fixed in a better
way.  The problem is that PRINTPREPARE sets the current buffer to the
super-duper secret buffer Vprin1_to_string_buffer (i.e., " prin1"), and
we should restore the real buffer before calling
`print-unreadable-function' in `print_vectorlike'.

But which buffer that was doesn't seem to be available at that point --
it's just stored in

#define PRINTPREPARE							\
   struct buffer *old = current_buffer;					\

So we'd need to store that in a way we can access it later, but I'm not
sure where/how it'd make the most sense to do so...  Any ideas?

Meanwhile I've pushed a failing test for this to the trunk.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Eli Zaretskii]

> Cc: 56773@debbugs.gnu.org
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 26 Jul 2022 14:31:19 +0200
> 
> So we'd need to store that in a way we can access it later, but I'm not
> sure where/how it'd make the most sense to do so...  Any ideas?

unwind_protect?

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> unwind_protect?

Where would the unwind_protect be?

The problem is here:

  if (!NILP (Vprint_unreadable_function)
      && FUNCTIONP (Vprint_unreadable_function))
    {
      specpdl_ref count = SPECPDL_INDEX ();
      /* Bind `print-unreadable-function' to nil to avoid accidental
	 infinite recursion in the function called.  */
      Lisp_Object func = Vprint_unreadable_function;
      specbind (Qprint_unreadable_function, Qnil);
      Lisp_Object result = CALLN (Ffuncall, func, obj,
				  escapeflag? Qt: Qnil);

We need to switch back to the original buffer before that Ffuncall, but
we don't know what the original function was -- it's just stored in a
local variable in prin1(-to-string).

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael_heerdegen@web.de,  56773@debbugs.gnu.org
> Date: Tue, 26 Jul 2022 14:44:31 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > unwind_protect?
> 
> Where would the unwind_protect be?
> 
> The problem is here:
> 
>   if (!NILP (Vprint_unreadable_function)
>       && FUNCTIONP (Vprint_unreadable_function))
>     {
>       specpdl_ref count = SPECPDL_INDEX ();
>       /* Bind `print-unreadable-function' to nil to avoid accidental
> 	 infinite recursion in the function called.  */
>       Lisp_Object func = Vprint_unreadable_function;
>       specbind (Qprint_unreadable_function, Qnil);
>       Lisp_Object result = CALLN (Ffuncall, func, obj,
> 				  escapeflag? Qt: Qnil);
> 
> We need to switch back to the original buffer before that Ffuncall, but
> we don't know what the original function was -- it's just stored in a
> local variable in prin1(-to-string).

I'm confused: "original function" or "original buffer"?

The original buffer is given by current_buffer before you call the
function which could change that, and the unwind_protect call should
be before calling that function.

But you already know all that, so I'm afraid I'm missing something
here.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm confused: "original function" or "original buffer"?

Original buffer.

> The original buffer is given by current_buffer before you call the
> function which could change that, and the unwind_protect call should
> be before calling that function.
>
> But you already know all that, so I'm afraid I'm missing something
> here.

Yes -- look at the call sequence from prin1-to-string and down to where
we're calling the function.  The buffer that was current when
prin1-to-string was called is not available from print_vectorlike, as
far as I can see.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yes -- look at the call sequence from prin1-to-string and down to where
> we're calling the function.  The buffer that was current when
> prin1-to-string was called is not available from print_vectorlike, as
> far as I can see.

That is -- there's two things that have to be fixed.  The first thing
(which I was talking about here) is that the current_buffer has to be
made available way down in print_vectorlike, so that we don't call the
callback from the " prin1" buffer, because that's really confusing.  I
think we can accomplish that by specbinding some variable in
PRINTPREPARE.

The other thing is that we have to ensure that the stuff in PRINTFINISH
is actually done when we have a non-local exit, and putting that stuff
in an unwind_protect should fix that.  I.e., PRINTPREPARE sets up the
unwind_protect, of course.

And when doing that, we might as well get rid of the macros altogether
and do this more traditionally via functions.

Does anybody see any obvious disadvantages to doing it this way?  I
guess it could potentially make calls to `prin1' (etc) slower, but I
don't know whether that'd be noticeable.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> unwind_protect?
>
> Where would the unwind_protect be?
>
> The problem is here:
>
>   if (!NILP (Vprint_unreadable_function)
>       && FUNCTIONP (Vprint_unreadable_function))
>     {
>       specpdl_ref count = SPECPDL_INDEX ();
>       /* Bind `print-unreadable-function' to nil to avoid accidental
> 	 infinite recursion in the function called.  */
>       Lisp_Object func = Vprint_unreadable_function;
>       specbind (Qprint_unreadable_function, Qnil);
>       Lisp_Object result = CALLN (Ffuncall, func, obj,
> 				  escapeflag? Qt: Qnil);
>
> We need to switch back to the original buffer before that Ffuncall, but
> we don't know what the original function was -- it's just stored in a
> local variable in prin1(-to-string).

Isn't that problem already present?  IOW, that function is still called
in the internal buffer under the current code.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

>> We need to switch back to the original buffer before that Ffuncall, but
>> we don't know what the original function was -- it's just stored in a
>> local variable in prin1(-to-string).
>
> Isn't that problem already present?  IOW, that function is still called
> in the internal buffer under the current code.

I'm not sure what you mean?  That's the problem I'm pointing out, and
asking for suggestions in how to fix.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Hello,
>
> emacs -Q:
>
> (progn (readablep (current-window-configuration))
>        (current-buffer))
> ==> #<buffer  prin1>
>
> After evaluating such a `readablep' call, weird things can happen
> because of this buffer being current.  I tried to actually display that
> buffer and the window I got looked quite funny, one second later Emacs
> had crashed.

Thanks, should be fixed now.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Po Lu via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs@gnu.org> writes:

> Thanks, should be fixed now.

The other bits in PRINTFINISH aren't done, though, are they?  For
instance, the

   if (free_print_buffer)						\
     {									\
       xfree (print_buffer);						\
       print_buffer = 0;						\
     }									\

bits might lead to a memory leak.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Po Lu via "Bug reports for GNU Emacs, the Swiss army knife of text
> editors" <bug-gnu-emacs@gnu.org> writes:
>
>> Thanks, should be fixed now.
>
> The other bits in PRINTFINISH aren't done, though, are they?  For
> instance, the
>
>    if (free_print_buffer)						\
>      {									\
>        xfree (print_buffer);						\
>        print_buffer = 0;						\
>      }									\
>
> bits might lead to a memory leak.

Right, thanks.  I fixed that as well.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Michael Heerdegen]

Po Lu <luangruo@yahoo.com> writes:

> Right, thanks.  I fixed that as well.

Thanks for the fix.  My use case seems to behave sane now.

Michael.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Right, thanks.  I fixed that as well.
>
> Thanks for the fix.  My use case seems to behave sane now.
>
> Michael.

Closing, thanks for testing.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Right, thanks.  I fixed that as well.

As I said earlier, I suspect that all of PRINTFINISH should go into the
unwind_protect form.  For instance, in the MARKERP situation, it seems
to want to adjust markers etc.  I have not analysed in detail, but those
code bits are presumably there for a reason, and may have to be run if
prin1 has done sometime (even if there's then been a non-local exit).

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> As I said earlier, I suspect that all of PRINTFINISH should go into the
> unwind_protect form.  For instance, in the MARKERP situation, it seems
> to want to adjust markers etc.  I have not analysed in detail, but those
> code bits are presumably there for a reason, and may have to be run if
> prin1 has done sometime (even if there's then been a non-local exit).

I don't think that's very intuitive, since signalling means "get me out
of here now".

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> I don't think that's very intuitive, since signalling means "get me out
> of here now".

But we shouldn't leave things in an inconsistent state, and I suspect
that we currently do.

By the way, did you look at the test failure your change led to?

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But we shouldn't leave things in an inconsistent state, and I suspect
> that we currently do.

I don't see any inconsistency, just the position of a marker being
changed.

> By the way, did you look at the test failure your change led to?

I didn't see one, but the spam filter is rather agressive these days, so
it would be nice for you to send it again in reply to this message.

Thanks.

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> I didn't see one, but the spam filter is rather agressive these days, so
> it would be nice for you to send it again in reply to this message.

Test terpri backtrace:
  signal(ert-test-failed (((should (string= (with-current-buffer (mark
  ert-fail(((should (string= (with-current-buffer (marker-buffer stand
  (if (unwind-protect (setq value-197 (apply fn-195 args-196)) (setq f
  (let (form-description-199) (if (unwind-protect (setq value-197 (app
  (let ((value-197 'ert-form-evaluation-aborted-198)) (let (form-descr
  (let* ((fn-195 #'string=) (args-196 (condition-case err (let ((signa
  (let ((standard-output (save-current-buffer (set-buffer (get-buffer-
  (closure (t) nil (let* ((fn-135 #'string=) (args-136 (condition-case
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name terpri :documentation nil :body (clos
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable))) #f(co
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable)))
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "-l" "src/print-tests.el" "--ev
  command-line()
  normal-top-level()
Test terpri condition:
    (ert-test-failed
     ((should
       (string=
	(with-current-buffer ... ...)
	"--------\n"))
      :form
      (string= "" "--------\n")
      :value nil :explanation
      (arrays-of-different-length 0 9 "" "--------\n" first-mismatch-at 0)))
   FAILED  41/45  terpri (0.000117 sec) at src/print-tests.el:110
   passed  42/45  test-dots (0.000062 sec)
   passed  43/45  test-prin1-overrides (0.000066 sec)
   passed  44/45  test-prin1-to-string-overrides (0.000049 sec)
   passed  45/45  test-unreadable (0.000029 sec)

Ran 45 tests, 44 results as expected, 1 unexpected (2022-07-28 13:08:34+0200, 0.132581 sec)
3 expected failures

1 unexpected results:
   FAILED  terpri

bug#56773: 29.0.50; (readablep UNREADABLE) causes strange things

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> I didn't see one, but the spam filter is rather agressive these days, so
>> it would be nice for you to send it again in reply to this message.
>
> Test terpri backtrace:
>   signal(ert-test-failed (((should (string= (with-current-buffer (mark
>   ert-fail(((should (string= (with-current-buffer (marker-buffer stand
>   (if (unwind-protect (setq value-197 (apply fn-195 args-196)) (setq f
>   (let (form-description-199) (if (unwind-protect (setq value-197 (app
>   (let ((value-197 'ert-form-evaluation-aborted-198)) (let (form-descr
>   (let* ((fn-195 #'string=) (args-196 (condition-case err (let ((signa
>   (let ((standard-output (save-current-buffer (set-buffer (get-buffer-
>   (closure (t) nil (let* ((fn-135 #'string=) (args-136 (condition-case
>   ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>   ert-run-test(#s(ert-test :name terpri :documentation nil :body (clos
>   ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
>   ert-run-tests((not (or (tag :expensive-test) (tag :unstable))) #f(co
>   ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable)))
>   ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
>   eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
>   command-line-1(("-L" ":." "-l" "ert" "-l" "src/print-tests.el" "--ev
>   command-line()
>   normal-top-level()
> Test terpri condition:
>     (ert-test-failed
>      ((should
>        (string=
> 	(with-current-buffer ... ...)
> 	"--------\n"))
>       :form
>       (string= "" "--------\n")
>       :value nil :explanation
>       (arrays-of-different-length 0 9 "" "--------\n" first-mismatch-at 0)))
>    FAILED  41/45  terpri (0.000117 sec) at src/print-tests.el:110
>    passed  42/45  test-dots (0.000062 sec)
>    passed  43/45  test-prin1-overrides (0.000066 sec)
>    passed  44/45  test-prin1-to-string-overrides (0.000049 sec)
>    passed  45/45  test-unreadable (0.000029 sec)
>
> Ran 45 tests, 44 results as expected, 1 unexpected (2022-07-28 13:08:34+0200, 0.132581 sec)
> 3 expected failures
>
> 1 unexpected results:
>    FAILED  terpri

Thanks, I see the problem now and will push a fix.