Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

First, we don't use "illegal" in these cases, we use "invalid":
there's nothing against any law in using bad values of some variables.

More importantly, why punting in these cases is the best possible
solution? why not fall back to some reasonable encoding instead?

And how come locale-coding-system is not a valid coding-system there
in the first place?

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> First, we don't use "illegal" in these cases, we use "invalid":
> there's nothing against any law in using bad values of some variables.

Okay, thanks.  I'll keep that in mind in the future.

> More importantly, why punting in these cases is the best possible
> solution? why not fall back to some reasonable encoding instead?

I think that if the locale coding system is set to an invalid value,
then the user probably knows there is a need to set it to something
other than the default value, but made a typo somewhere.

Decoding multibyte X keyboard input and XIM pre-edit text with any
coding system other than the actual locale coding system will result in
nonsensical text.

Maybe the best thing to do would be to log a message stating that the
specified coding system is invalid.

> And how come locale-coding-system is not a valid coding-system there
> in the first place?

The user could have set it to an invalid value, which leads to
spectacular crashes when setup_coding_system signals inside a GTK event
filter.

Thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 13 Feb 2022 21:16:19 +0800
> 
> Decoding multibyte X keyboard input and XIM pre-edit text with any
> coding system other than the actual locale coding system will result in
> nonsensical text.

But even nonsensical text is better than no text at all, don't you
agree?

And anyway, in what possible encodings could this text be?  Can't we
detect the encoding with reasonable success rate?

I mean, dropping input on the floor, let alone doing that silently, is
pretty rough, no?

> > And how come locale-coding-system is not a valid coding-system there
> > in the first place?
> 
> The user could have set it to an invalid value, which leads to
> spectacular crashes when setup_coding_system signals inside a GTK event
> filter.

Can't we catch those signals?

If we cannot catch these signals, it probably means it is unsafe to
call decoding routines in that place anyway.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> But even nonsensical text is better than no text at all, don't you
> agree?
>
> And anyway, in what possible encodings could this text be?  Can't we
> detect the encoding with reasonable success rate?

We could try the default value of locale-coding-system, but see below.

> I mean, dropping input on the floor, let alone doing that silently, is
> pretty rough, no?

It could be accompanied with a message that the locale coding system is
invalid.

Most keyboard input isn't dropped.  The only keyboard input that is is
multibyte text from input methods, so it won't affect the user's ability
to fix the locale coding system.

> Can't we catch those signals?

Not within C code, I think.

Thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Mon, 14 Feb 2022 08:51:17 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But even nonsensical text is better than no text at all, don't you
> > agree?
> >
> > And anyway, in what possible encodings could this text be?  Can't we
> > detect the encoding with reasonable success rate?
> 
> We could try the default value of locale-coding-system

That should be done in any case, I think.

> > I mean, dropping input on the floor, let alone doing that silently, is
> > pretty rough, no?
> 
> It could be accompanied with a message that the locale coding system is
> invalid.
> 
> Most keyboard input isn't dropped.  The only keyboard input that is is
> multibyte text from input methods, so it won't affect the user's ability
> to fix the locale coding system.

I'd rather we tried to decode it in any way possible.  We never do
anything like this anywhere else where decoding is involved.

> > Can't we catch those signals?
> 
> Not within C code, I think.

I don't understand why.  We do that in C in many places, see the calls
to internal_condition_case and its ilk.  Or am I missing something?

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> I'd rather we tried to decode it in any way possible.  We never do
> anything like this anywhere else where decoding is involved.

Thanks, could you point me to an example of decoding text "in any way
possible?  I'm not sure how to do that inside C code.

> I don't understand why.  We do that in C in many places, see the calls
> to internal_condition_case and its ilk.  Or am I missing something?

I was under the impression that `internal_condition_case' could only be
called for Lisp functions, but that's evidently wrong.

Thanks in advance.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Mon, 14 Feb 2022 12:46:56 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'd rather we tried to decode it in any way possible.  We never do
> > anything like this anywhere else where decoding is involved.
> 
> Thanks, could you point me to an example of decoding text "in any way
> possible?  I'm not sure how to do that inside C code.

See DECODE_SYSTEM, for one simple example.  Can you use that?

> > I don't understand why.  We do that in C in many places, see the calls
> > to internal_condition_case and its ilk.  Or am I missing something?
> 
> I was under the impression that `internal_condition_case' could only be
> called for Lisp functions, but that's evidently wrong.

Yes, we use that for C functions as well.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> [Why private email?]

Sorry for that, I copied in the list again.  And I apologize for sending
all of this twice.

> But this means we cannot call the decoding stuff there at all, because
> some coding-systems call into Lisp, and Lisp can signal an error.  So
> we probably should take the raw bytes in that place, record them in
> the event queue, and then decode it later, when it's safe.

What safe place would you find appropriate for the job?  And I'm not
sure if we have a mechanism in place to record the raw bytes that way,
so one will have to be devised.

> Are there other situations where we decode stuff inside
> handle_one_xevent or in a XIM callback?

There aren't any, AFAICT, other than the ones that I changed.

> If so, and if what you write is really accurate, then we have a
> ticking bomb waiting to go off, and we should defuse it ASAP.

Indeed.

> But given what you say above I think we have a much more serious
> problem on our hands.

Based on what you have said, I agree.

Thanks in advance.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Tue, 15 Feb 2022 21:55:54 +0800
> 
> > But this means we cannot call the decoding stuff there at all, because
> > some coding-systems call into Lisp, and Lisp can signal an error.  So
> > we probably should take the raw bytes in that place, record them in
> > the event queue, and then decode it later, when it's safe.
> 
> What safe place would you find appropriate for the job?

Where we read input in keyboard.c, for example.  Basically, any time
we are in the "normal" context of the main thread and in the Emacs
command loop.

> And I'm not sure if we have a mechanism in place to record the raw
> bytes that way, so one will have to be devised.

The event queue has members that are Lisp objects; can't you use them
to record unibyte strings?

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Where we read input in keyboard.c, for example.  Basically, any time
> we are in the "normal" context of the main thread and in the Emacs
> command loop.

Thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> Where we read input in keyboard.c, for example.  Basically, any time
>> we are in the "normal" context of the main thread and in the Emacs
>> command loop.
>
> Thanks.

I installed a change on master that should avert this problem.  I
couldn't find a way to decode the XIM callback data inside keyboard.c,
but handling the errors with internal_condition_case seems to work.

Thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> I installed a change on master that should avert this problem.  I
> couldn't find a way to decode the XIM callback data inside keyboard.c,
> but handling the errors with internal_condition_case seems to work.

BTW, the same problem exists on the release branch when built with
recent versions of GLib.

Would it be worth fixing the bug there as well?  Thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Wed, 16 Feb 2022 13:12:31 +0800
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> > I installed a change on master that should avert this problem.  I
> > couldn't find a way to decode the XIM callback data inside keyboard.c,
> > but handling the errors with internal_condition_case seems to work.
> 
> BTW, the same problem exists on the release branch when built with
> recent versions of GLib.
> 
> Would it be worth fixing the bug there as well?  Thanks.

Only if this is a recent problem.  Otherwise, I'd prefer not to
introduce such non-trivial code into the release branch so late into
the pretest.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Only if this is a recent problem.  Otherwise, I'd prefer not to
> introduce such non-trivial code into the release branch so late into
> the pretest.

It's a problem that cropped up in GLib after 27.1 was released, but I
don't know if that counts as recent enough.

Perhaps we could install the simpler fix that avoids passing an invalid
coding system to setup_coding_system on the release branch, which should
cover botched values of `locale-coding-system'.

Thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Wed, 16 Feb 2022 20:33:08 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Only if this is a recent problem.  Otherwise, I'd prefer not to
> > introduce such non-trivial code into the release branch so late into
> > the pretest.
> 
> It's a problem that cropped up in GLib after 27.1 was released, but I
> don't know if that counts as recent enough.

Emacs 27.1 was release 1.5 years ago.  If these problems are real, we
should have heard about them by now.

> Perhaps we could install the simpler fix that avoids passing an invalid
> coding system to setup_coding_system on the release branch, which should
> cover botched values of `locale-coding-system'.

That's too general a fix in a place that's too low-level to know what
is TRT in these cases.  It would prevent us from signaling an error
when a Lisp program uses an invalid encoding, for example -- these
problems cannot be silently ignored.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> That's too general a fix in a place that's too low-level to know what
> is TRT in these cases.  It would prevent us from signaling an error
> when a Lisp program uses an invalid encoding, for example -- these
> problems cannot be silently ignored.

I was specifically speaking about changing the call in
handle_one_xevent, not changing the behavior of setup_coding_system in
general.

Thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Wed, 16 Feb 2022 21:05:16 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > That's too general a fix in a place that's too low-level to know what
> > is TRT in these cases.  It would prevent us from signaling an error
> > when a Lisp program uses an invalid encoding, for example -- these
> > problems cannot be silently ignored.
> 
> I was specifically speaking about changing the call in
> handle_one_xevent, not changing the behavior of setup_coding_system in
> general.

I guess I don't understand what you want to install.  Can you show a
patch?

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> I guess I don't understand what you want to install.  Can you show a
> patch?

Sure, thanks:

diff --git a/src/xterm.c b/src/xterm.c
index b55a54b945..9fa725e9f3 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -8734,6 +8734,15 @@ handle_one_xevent (struct x_display_info *dpyinfo,
 	      {
 		/* Decode the input data.  */
 
+#ifdef HAVE_GLIB
+		/* If this isn't done in a build with GLib (usually
+		   with GTK), then the resulting signal in
+		   `setup_coding_system' will cause Emacs to
+		   crash.  */
+		if (NILP (Fcoding_system_p (coding_system)))
+		  goto done_keysym;
+#endif
+
 		/* The input should be decoded with `coding_system'
 		   which depends on which X*LookupString function
 		   we used just above and the locale.  */

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Wed, 16 Feb 2022 21:40:57 +0800
> 
> --- a/src/xterm.c
> +++ b/src/xterm.c
> @@ -8734,6 +8734,15 @@ handle_one_xevent (struct x_display_info *dpyinfo,
>  	      {
>  		/* Decode the input data.  */
>  
> +#ifdef HAVE_GLIB
> +		/* If this isn't done in a build with GLib (usually
> +		   with GTK), then the resulting signal in
> +		   `setup_coding_system' will cause Emacs to
> +		   crash.  */
> +		if (NILP (Fcoding_system_p (coding_system)))
> +		  goto done_keysym;
> +#endif
> +
>  		/* The input should be decoded with `coding_system'
>  		   which depends on which X*LookupString function
>  		   we used just above and the locale.  */

This isn't right, IMO.  You should decode using Qraw_text in the
problematic case.  Dropping the bytes on the floor is too radical,
certainly for the release branch.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> This isn't right, IMO.  You should decode using Qraw_text in the
> problematic case.  Dropping the bytes on the floor is too radical,
> certainly for the release branch.

How about something like this?

diff --git a/src/xterm.c b/src/xterm.c
index b55a54b945..53d89240f1 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -8734,6 +8734,15 @@ handle_one_xevent (struct x_display_info *dpyinfo,
 	      {
 		/* Decode the input data.  */
 
+#ifdef HAVE_GLIB
+		/* If this isn't done in a build with GLib (usually
+		   with GTK), then the resulting signal in
+		   `setup_coding_system' will cause Emacs to
+		   crash.  */
+		if (NILP (Fcoding_system_p (coding_system)))
+		  coding_system = Qraw_text;
+#endif
+
 		/* The input should be decoded with `coding_system'
 		   which depends on which X*LookupString function
 		   we used just above and the locale.  */

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 17 Feb 2022 08:45:19 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > This isn't right, IMO.  You should decode using Qraw_text in the
> > problematic case.  Dropping the bytes on the floor is too radical,
> > certainly for the release branch.
> 
> How about something like this?
> 
> diff --git a/src/xterm.c b/src/xterm.c
> index b55a54b945..53d89240f1 100644
> --- a/src/xterm.c
> +++ b/src/xterm.c
> @@ -8734,6 +8734,15 @@ handle_one_xevent (struct x_display_info *dpyinfo,
>  	      {
>  		/* Decode the input data.  */
>  
> +#ifdef HAVE_GLIB
> +		/* If this isn't done in a build with GLib (usually
> +		   with GTK), then the resulting signal in
> +		   `setup_coding_system' will cause Emacs to
> +		   crash.  */
> +		if (NILP (Fcoding_system_p (coding_system)))
> +		  coding_system = Qraw_text;
> +#endif

Yes, that's what I had in mind.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes, that's what I had in mind.

Does that mean it's okay to install that on the release branch?

Thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 17 Feb 2022 14:51:21 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes, that's what I had in mind.
> 
> Does that mean it's okay to install that on the release branch?

Yes, thanks.

Re: master 6ed1994d27: Prevent crashes from illegal locale coding systems

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes, thanks.

Now done, thanks.