bug#62009: 29.0.60; Emacs crashes on setf symbol-name

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

Execute the following in the scratch buffer:

(setf (aref (symbol-name 'car) 1) ?o)

Emacs crashes with a segmentation fault. Is this a well-known issue? I
could reproduce the problem on Emacs 27 and 29. Should there be some
mechanism to protect the strings of symbols?

I found the snippet on reddit:
https://old.reddit.com/r/emacs/comments/11ix6yu/ive_found_what_ive_been_looking_for/jb4ah5v/

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

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

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


Daniel Mendler <mail@daniel-mendler.de> writes:

> Execute the following in the scratch buffer:
>
> (setf (aref (symbol-name 'car) 1) ?o)
>
> Emacs crashes with a segmentation fault. Is this a well-known issue? I
> could reproduce the problem on Emacs 27 and 29. Should there be some
> mechanism to protect the strings of symbols?
>
> I found the snippet on reddit:
> https://old.reddit.com/r/emacs/comments/11ix6yu/ive_found_what_ive_been_looking_for/jb4ah5v/

Can't access reddit, but can reproduce in recent master (6fb8a4dff7ef).

To test, first put this file under emacs.git/src/:


[-- Attachment #2: test.el --]
[-- Type: text/plain, Size: 147 bytes --]

(defun foo (symbol)
  (message "[1] %S" (symbol-name symbol))
  (setf (aref (symbol-name symbol) 1) ?x)
  (message "[2] %S" (symbol-name symbol)))

[-- Attachment #3: Type: text/plain, Size: 1203 bytes --]


    $ make; cd src

Then do the following for each symbol:
                - setf
                - find-file
                - with-current-buffer
                - buffer-file-name

    $ ./emacs -Q -batch -l test.el -eval '(foo (quote setf))'
    [1] "setf"
    [2] "sxtf"

    $ ./emacs -Q -batch -l test.el -eval '(foo (quote find-file))'
    [1] "find-file"
    [2] "fxnd-file"

And these below below: aref, null, car, cdr, save-current-buffer

    $ ./emacs -Q -batch -l test.el -eval '(foo (quote aref))'
    [1] "aref"
    Fatal error 11: Segmentation fault
    Backtrace:
    ...

My observation is that symbols "introduced" via C defuns and defmacros
exhibit this problem, whereas those introduced via Elisp defuns and
defmacros do not.  No symbols introduced via defvars exhibit this
problem, as shown above with buffer-file-name.

Seeing that it is a segfault, maybe the setf is trying to modify
readonly memory produced by the C defuns and defmacros?  If that is the
case, *if* we allow such modifications, we should make the memory
readwrite; *otherwise* maybe we should no-op, warn, or err in setf and
friends when we see readonly memory blocks?

With this collection of GDB commands:


[-- Attachment #4: debug.gdb --]
[-- Type: text/plain, Size: 39 bytes --]

set debuginfod enabled off
run
bt
exit

[-- Attachment #5: Type: text/plain, Size: 236 bytes --]


And this GDB command line option:

    $ gdb -x debug.gdb --batch --args ./emacs -Q -batch -l ../test.el -eval '(foo (quote car))' > car.backtrace

I get the backtrace (attached below) for setf + symbol-name + 'car as
reported by OP.


[-- Attachment #6: car.backtrace --]
[-- Type: text/plain, Size: 3048 bytes --]

SIGINT is used by the debugger.
Are you sure you want to change it? (y or n) [answered Y; input not from terminal]
DISPLAY = :0
TERM = xterm-256color
Breakpoint 1 at 0x4e2ab: file emacs.c, line 426.
Breakpoint 2 at 0x142970: file xterm.c, line 26474.
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
Faset (array=XIL(0x7ffff1d10adc), idx=make_fixnum(1), newelt=make_fixnum(120)) at /opt/src/emacs/base/62009-setf-symbol-name/src/lisp.h:1671
1671      return XSTRING (string)->u.s.data;
#0  Faset (array=XIL(0x7ffff1d10adc), idx=make_fixnum(1), newelt=make_fixnum(120)) at /opt/src/emacs/base/62009-setf-symbol-name/src/lisp.h:1671
#1  0x000055555576f431 in eval_sub (form=<optimized out>) at eval.c:2506
#2  0x000055555577132d in Fprogn (body=XIL(0)) at eval.c:436
#3  FletX (args=<optimized out>) at eval.c:958
#4  0x000055555576f198 in eval_sub (form=<optimized out>) at eval.c:2451
#5  0x00005555557701ed in Fprogn (body=XIL(0x7ffff24d25d3)) at eval.c:436
#6  funcall_lambda (fun=<optimized out>, fun@entry=XIL(0x7ffff24d5b13), nargs=nargs@entry=1, arg_vector=arg_vector@entry=0x7fffffffd9e0) at eval.c:3235
#7  0x000055555577095c in apply_lambda (fun=fun@entry=XIL(0x7ffff24d5b13), args=<optimized out>, count=count@entry=...) at eval.c:3105
#8  0x000055555576eea6 in eval_sub (form=form@entry=XIL(0x7ffff24d5cf3)) at eval.c:2590
#9  0x0000555555771a8f in Feval (form=XIL(0x7ffff24d5cf3), lexical=<optimized out>) at eval.c:2363
#10 0x00005555557b5882 in exec_byte_code (fun=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>) at bytecode.c:809
#11 0x000055555577095c in apply_lambda (fun=fun@entry=XIL(0x7ffff19dc085), args=<optimized out>, count=count@entry=...) at eval.c:3105
#12 0x000055555576eea6 in eval_sub (form=form@entry=XIL(0x7ffff1e13773)) at eval.c:2590
#13 0x0000555555771a8f in Feval (form=XIL(0x7ffff1e13773), lexical=<optimized out>) at eval.c:2363
#14 0x0000555555769cf7 in internal_condition_case (bfun=bfun@entry=0x5555556da3b0 <top_level_2>, handlers=handlers@entry=XIL(0x90), hfun=hfun@entry=0x5555556e1990 <cmd_error>) at eval.c:1474
#15 0x00005555556dad36 in top_level_1 (ignore=ignore@entry=XIL(0)) at keyboard.c:1141
#16 0x0000555555769c51 in internal_catch (tag=tag@entry=XIL(0x103e0), func=func@entry=0x5555556dad10 <top_level_1>, arg=arg@entry=XIL(0)) at eval.c:1197
#17 0x00005555556da32f in command_loop () at keyboard.c:1101
#18 0x00005555556e1512 in recursive_edit_1 () at keyboard.c:711
#19 0x00005555556e18a0 in Frecursive_edit () at keyboard.c:794
#20 0x00005555555ab49f in main (argc=7, argv=0x7fffffffe0a8) at emacs.c:2530

Lisp Backtrace:
"aset" (0xffffd790)
"let*" (0xffffd8c0)
"foo" (0xffffd9e0)
"eval" (0xf05ff1c0)
"command-line-1" (0xf05ff0b8)
"command-line" (0xf05ff040)
"normal-top-level" (0xffffdbd0)
A debugging session is active.

    Inferior 1 [process 1106482] will be killed.

Quit anyway? (y or n) [answered Y; input not from terminal]

[-- Attachment #7: Type: text/plain, Size: 21 bytes --]


HTH.

--
Best,


RY

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Michael Heerdegen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Execute the following in the scratch buffer:
>
> (setf (aref (symbol-name 'car) 1) ?o)
>
> Emacs crashes with a segmentation fault. Is this a well-known issue? I
> could reproduce the problem on Emacs 27 and 29. Should there be some
> mechanism to protect the strings of symbols?

I vaguely remember this has been discussed some time ago, but I don't
find a bug report about it.  Maybe it had been on emacs-dev.

Maybe the outcome was something like that we can't protect everybody in
every case from shooting in the own foot, I don't recall.

Michael.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

On 3/7/23 16:45, Michael Heerdegen wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
> 
>> Execute the following in the scratch buffer:
>>
>> (setf (aref (symbol-name 'car) 1) ?o)
>>
>> Emacs crashes with a segmentation fault. Is this a well-known issue? I
>> could reproduce the problem on Emacs 27 and 29. Should there be some
>> mechanism to protect the strings of symbols?
>
> Maybe the outcome was something like that we can't protect everybody in
> every case from shooting in the own foot, I don't recall.

Maybe it would be possible to introduce a flag which marks strings as
"frozen"? Then we could ensure that no mutations of such frozen string
happen. Freezing strings (vectors or pairs) may be generally useful
beyond preventing such issues.

Daniel

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 62009@debbugs.gnu.org
> Date: Tue, 7 Mar 2023 18:08:43 +0100
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> On 3/7/23 16:45, Michael Heerdegen wrote:
> > Daniel Mendler <mail@daniel-mendler.de> writes:
> > 
> >> Execute the following in the scratch buffer:
> >>
> >> (setf (aref (symbol-name 'car) 1) ?o)
> >>
> >> Emacs crashes with a segmentation fault. Is this a well-known issue? I
> >> could reproduce the problem on Emacs 27 and 29. Should there be some
> >> mechanism to protect the strings of symbols?
> >
> > Maybe the outcome was something like that we can't protect everybody in
> > every case from shooting in the own foot, I don't recall.
> 
> Maybe it would be possible to introduce a flag which marks strings as
> "frozen"? Then we could ensure that no mutations of such frozen string
> happen.

AFAICT, they _are_ frozen.  These names are in read-only memory, where
you cannot write.  That's why Emacs crashes, AFAIU: the code is trying
to write to protected memory.

Just don't do that, cause it's gonna hurt...

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 62009@debbugs.gnu.org
>> Date: Tue, 7 Mar 2023 18:08:43 +0100
>> From: Daniel Mendler <mail@daniel-mendler.de>
>> 
>> On 3/7/23 16:45, Michael Heerdegen wrote:
>> > Daniel Mendler <mail@daniel-mendler.de> writes:
>> > 
>> >> Execute the following in the scratch buffer:
>> >>
>> >> (setf (aref (symbol-name 'car) 1) ?o)
>> >>
>> >> Emacs crashes with a segmentation fault. Is this a well-known issue? I
>> >> could reproduce the problem on Emacs 27 and 29. Should there be some
>> >> mechanism to protect the strings of symbols?
>> >
>> > Maybe the outcome was something like that we can't protect everybody in
>> > every case from shooting in the own foot, I don't recall.
>> 
>> Maybe it would be possible to introduce a flag which marks strings as
>> "frozen"? Then we could ensure that no mutations of such frozen string
>> happen.
>
> AFAICT, they _are_ frozen.  These names are in read-only memory, where
> you cannot write.  That's why Emacs crashes, AFAIU: the code is trying
> to write to protected memory.
>
> Just don't do that, cause it's gonna hurt...

Is it not possible to detect this before the illegal memory access, and
raise a signal in Emacs Lisp?

-- 
Philip Kaludercic

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Daniel Mendler <mail@daniel-mendler.de>,  michael_heerdegen@web.de,
>   monnier@iro.umontreal.ca,  62009@debbugs.gnu.org
> Date: Thu, 09 Mar 2023 21:11:13 +0000
> 
> > AFAICT, they _are_ frozen.  These names are in read-only memory, where
> > you cannot write.  That's why Emacs crashes, AFAIU: the code is trying
> > to write to protected memory.
> >
> > Just don't do that, cause it's gonna hurt...
> 
> Is it not possible to detect this before the illegal memory access, and
> raise a signal in Emacs Lisp?

It won't be easy, if at all possible.  And I'm not sure we even want
to do that.  What would be the purpose of supporting such a use of
Emacs?

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Augusto Stoffel]

On Fri, 10 Mar 2023 at 09:11, Eli Zaretskii wrote:

>> Is it not possible to detect this before the illegal memory access, and
>> raise a signal in Emacs Lisp?
>
> It won't be easy, if at all possible.  And I'm not sure we even want
> to do that.  What would be the purpose of supporting such a use of
> Emacs?

What is the purpose of supporting mutation of symbol names in general?

  (aset (symbol-name 'find-file) 1 ?o)
  (fboundp 'find-file)
  => nil

This one doesn't crash Emacs, but wreaks havoc, maybe in even worse
ways.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Augusto Stoffel]

On Fri, 10 Mar 2023 at 09:45, Augusto Stoffel wrote:

> On Fri, 10 Mar 2023 at 09:11, Eli Zaretskii wrote:
>
>>> Is it not possible to detect this before the illegal memory access, and
>>> raise a signal in Emacs Lisp?
>>
>> It won't be easy, if at all possible.  And I'm not sure we even want
>> to do that.  What would be the purpose of supporting such a use of
>> Emacs?
>
> What is the purpose of supporting mutation of symbol names in general?
>
>   (aset (symbol-name 'find-file) 1 ?o)
>   (fboundp 'find-file)
>   => nil
>
> This one doesn't crash Emacs, but wreaks havoc, maybe in even worse
> ways.

(To clarify, I think this of course should raise a signal.)

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: Philip Kaludercic <philipk@posteo.net>,  michael_heerdegen@web.de,
>   mail@daniel-mendler.de,  monnier@iro.umontreal.ca,  62009@debbugs.gnu.org
> Date: Fri, 10 Mar 2023 09:47:33 +0100
> 
> On Fri, 10 Mar 2023 at 09:45, Augusto Stoffel wrote:
> 
> > On Fri, 10 Mar 2023 at 09:11, Eli Zaretskii wrote:
> >
> >>> Is it not possible to detect this before the illegal memory access, and
> >>> raise a signal in Emacs Lisp?
> >>
> >> It won't be easy, if at all possible.  And I'm not sure we even want
> >> to do that.  What would be the purpose of supporting such a use of
> >> Emacs?
> >
> > What is the purpose of supporting mutation of symbol names in general?
> >
> >   (aset (symbol-name 'find-file) 1 ?o)
> >   (fboundp 'find-file)
> >   => nil
> >
> > This one doesn't crash Emacs, but wreaks havoc, maybe in even worse
> > ways.
> 
> (To clarify, I think this of course should raise a signal.)

Why bother?  Emacs is not in the business of preventing Lisp
programmers from shooting themselves in the foot, certainly not when
that incurs runtime overhead, even a small one.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

On 3/10/23 12:50, Eli Zaretskii wrote:
> Why bother?  Emacs is not in the business of preventing Lisp
> programmers from shooting themselves in the foot, certainly not when
> that incurs runtime overhead, even a small one.

Of course Elisp is in the business of preventing programmers from
shooting themselves in the foot, otherwise we would extend Emacs in C.
It is not only that Elisp is easier to write thanks to macros and other
conveniences, but also because it is safer!

I fully agree with you that we should not introduce a performance
regression, in particular not one which increases GC pressure badly.
Furthermore I agree that this is a minor bug which only occurs as an
edge case when some specific strings are mutated.

However the cost of fixing this bug is minor, since Elisp already
supports read-only data stuctures as pointed out by Robert. We may only
need an additional check in aset which won't introduce significant costs
thanks to the branch predictor of CPUs. As Augusto pointed out, aset on
strings can already be very costly because the string must be
reallocated. In comparison, the additional check will be free.

Daniel

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Date: Fri, 10 Mar 2023 13:00:34 +0100
> Cc: philipk@posteo.net, michael_heerdegen@web.de, monnier@iro.umontreal.ca,
>  62009@debbugs.gnu.org, Robert Pluim <rpluim@gmail.com>,
>  Augusto Stoffel <arstoffel@gmail.com>
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> On 3/10/23 12:50, Eli Zaretskii wrote:
> > Why bother?  Emacs is not in the business of preventing Lisp
> > programmers from shooting themselves in the foot, certainly not when
> > that incurs runtime overhead, even a small one.
> 
> Of course Elisp is in the business of preventing programmers from
> shooting themselves in the foot, otherwise we would extend Emacs in C.

We disagree here, and this is a very fundamental disagreement, which
basically means continuing this argument is pointless, since we have
no common basis.

> I fully agree with you that we should not introduce a performance
> regression, in particular not one which increases GC pressure badly.
> Furthermore I agree that this is a minor bug which only occurs as an
> edge case when some specific strings are mutated.
> 
> However the cost of fixing this bug is minor

No, it isn't, not in my book.

Sorry, I object to any change to cater for this use case.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

On 3/10/23 13:35, Eli Zaretskii wrote:
>> Date: Fri, 10 Mar 2023 13:00:34 +0100
>> Cc: philipk@posteo.net, michael_heerdegen@web.de, monnier@iro.umontreal.ca,
>>  62009@debbugs.gnu.org, Robert Pluim <rpluim@gmail.com>,
>>  Augusto Stoffel <arstoffel@gmail.com>
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>> On 3/10/23 12:50, Eli Zaretskii wrote:
>>> Why bother?  Emacs is not in the business of preventing Lisp
>>> programmers from shooting themselves in the foot, certainly not when
>>> that incurs runtime overhead, even a small one.
>>
>> Of course Elisp is in the business of preventing programmers from
>> shooting themselves in the foot, otherwise we would extend Emacs in C.
> 
> We disagree here, and this is a very fundamental disagreement, which
> basically means continuing this argument is pointless, since we have
> no common basis.

I don't see that the disagreement is that strong. For example aset
signals an error if you try to access elements out of bounds.

(aset "abc" 3 ?x) -> args-out-of-range

So there are clearly use cases where signaling an error is justified. In
other cases you claim signaling an error is unjustified and a crash is
better. I don't like the crashing. That's the whole disagreement. I
suspect that you also don't like if Emacs crashes. Maybe it doesn't
bother you in this case, but in others.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Date: Fri, 10 Mar 2023 13:45:11 +0100
> Cc: philipk@posteo.net, michael_heerdegen@web.de, monnier@iro.umontreal.ca,
>  62009@debbugs.gnu.org, rpluim@gmail.com, arstoffel@gmail.com
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> > We disagree here, and this is a very fundamental disagreement, which
> > basically means continuing this argument is pointless, since we have
> > no common basis.
> 
> I don't see that the disagreement is that strong. For example aset
> signals an error if you try to access elements out of bounds.
> 
> (aset "abc" 3 ?x) -> args-out-of-range

yes, because that's a frequent programmatic mistake.

> So there are clearly use cases where signaling an error is justified.

Of course.  It's just that the use case being discussed is not one of
them.

> In other cases you claim signaling an error is unjustified and a
> crash is better.

I said nothing of the kind.  I said it was unjustified in the
particular case which is being discussed here.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

On 3/10/23 13:57, Eli Zaretskii wrote:
>> Date: Fri, 10 Mar 2023 13:45:11 +0100
>> Cc: philipk@posteo.net, michael_heerdegen@web.de, monnier@iro.umontreal.ca,
>>  62009@debbugs.gnu.org, rpluim@gmail.com, arstoffel@gmail.com
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>>> We disagree here, and this is a very fundamental disagreement, which
>>> basically means continuing this argument is pointless, since we have
>>> no common basis.
>>
>> I don't see that the disagreement is that strong. For example aset
>> signals an error if you try to access elements out of bounds.
>>
>> (aset "abc" 3 ?x) -> args-out-of-range
> 
> yes, because that's a frequent programmatic mistake.

Agree.

>> So there are clearly use cases where signaling an error is justified.
> 
> Of course.  It's just that the use case being discussed is not one of
> them.

I agree with you, that this is not a common mistake. But it is still a
mistake and we could easily protect the user from it.

This is a general question - do we want to prevent and catch most
programmer mistakes or not? You seem to lean on rather not catching some
errors which are rare and I am in favor of catching more errors if the
costs permit. In this case, the costs are small in my book (I cannot
look into your book and understand how you come to your conclusions).
Given that Robert already pointed out that objects can be marked as
read-only, all the necessary infrastructure is already in place, making
this a cheap change.

Catching more mistakes improves overall robustness of Emacs and
generally there are also security considerations which should be taken
into account. It may not matter in this case, but ensuring that the
Elisp runtime is memory safe as much as possible is a worthy goal.

>> In other cases you claim signaling an error is unjustified and a
>> crash is better.
> 
> I said nothing of the kind.  I said it was unjustified in the
> particular case which is being discussed here.

You clearly said that you object to any measures which fix this issue.
This means you prefer if Emacs crashes, instead of it signaling an error.

Daniel

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Date: Fri, 10 Mar 2023 14:08:44 +0100
> Cc: philipk@posteo.net, michael_heerdegen@web.de, monnier@iro.umontreal.ca,
>  62009@debbugs.gnu.org, rpluim@gmail.com, arstoffel@gmail.com
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> >> (aset "abc" 3 ?x) -> args-out-of-range
> > 
> > yes, because that's a frequent programmatic mistake.
> 
> Agree.
> 
> >> So there are clearly use cases where signaling an error is justified.
> > 
> > Of course.  It's just that the use case being discussed is not one of
> > them.
> 
> I agree with you, that this is not a common mistake. But it is still a
> mistake and we could easily protect the user from it.

I don't agree with "easily".  And I think "not common" is an
understatement.

> This is a general question - do we want to prevent and catch most
> programmer mistakes or not?

Depends on the mistakes and the price.

> You seem to lean on rather not catching some errors which are rare
> and I am in favor of catching more errors if the costs permit.

We disagree about the importance of the mistake, and we disagree about
the costs of handling it.

In addition to the runtime costs, in terms of CPU and memory/GC,
there's also the aspect of introducing into Emacs a feature that we
will have to support for the observable future.  What if we want at
some point to change how these strings are store, and that change
makes these mistakes even harder to support?  We are taking upon
ourselves an obligation that we could regret, for the benefit of silly
code that shouldn't be written in the first place.  That kind of
trade-off makes no sense.

> Catching more mistakes improves overall robustness of Emacs and
> generally there are also security considerations which should be taken
> into account. It may not matter in this case, but ensuring that the
> Elisp runtime is memory safe as much as possible is a worthy goal.

The disagreement is not about principles, it's about this particular
case.

> >> In other cases you claim signaling an error is unjustified and a
> >> crash is better.
> > 
> > I said nothing of the kind.  I said it was unjustified in the
> > particular case which is being discussed here.
> 
> You clearly said that you object to any measures which fix this issue.
> This means you prefer if Emacs crashes, instead of it signaling an error.

That's your conclusion, not something I said.  What I did say is that
the nature and the rarity of the issue do not justify the costs.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Gregory Heytings]

>
> Sorry, I object to any change to cater for this use case.
>

IMO a reasonable change here would be to update the docstring of 
symbol-name, which only says "Return SYMBOL's name, a string.", with a 
warning similar to the one in the manual:

Warning: Changing the string by substituting characters does change the 
name of the symbol, but fails to update the obarray, so don't do it!

Perhaps we could also explicitly mention, in the docstring and/or in the 
manual, that doing that can also crash Emacs.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Date: Sat, 11 Mar 2023 15:16:36 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Daniel Mendler <mail@daniel-mendler.de>, philipk@posteo.net, 
>     michael_heerdegen@web.de, rpluim@gmail.com, monnier@iro.umontreal.ca, 
>     62009@debbugs.gnu.org, arstoffel@gmail.com
> 
> 
> >
> > Sorry, I object to any change to cater for this use case.
> >
> 
> IMO a reasonable change here would be to update the docstring of 
> symbol-name, which only says "Return SYMBOL's name, a string.", with a 
> warning similar to the one in the manual:
> 
> Warning: Changing the string by substituting characters does change the 
> name of the symbol, but fails to update the obarray, so don't do it!

That is okay, of course.  My objection was to the code changes.

> Perhaps we could also explicitly mention, in the docstring and/or in the 
> manual, that doing that can also crash Emacs.

I think being a bit vague here and talking about "dangers" should be
good enough, since not every such code will crash, and probably also
not on every platform and in every build configuration.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Gregory Heytings]

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


>
> I think being a bit vague here and talking about "dangers" should be 
> good enough, since not every such code will crash, and probably also not 
> on every platform and in every build configuration.
>

Is the attached patch okay?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Improve-warning-about-changing-the-string-returned-b.patch --]
[-- Type: text/x-diff; name=Improve-warning-about-changing-the-string-returned-b.patch, Size: 1689 bytes --]

From 32f0fe0aa5fce7fd71f80a166676ef3e5f1e7b32 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 18 Mar 2023 22:41:33 +0000
Subject: [PATCH] Improve warning about changing the string returned by
 symbol-name

* src/data.c (Fsymbol_name): Add warning.

* doc/lispref/symbols.texi (Creating Symbols): Improve warning.
---
 doc/lispref/symbols.texi | 5 ++---
 src/data.c               | 5 ++++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
index 5b53cbe310a..c6a0408abd1 100644
--- a/doc/lispref/symbols.texi
+++ b/doc/lispref/symbols.texi
@@ -276,9 +276,8 @@ Creating Symbols
 @end group
 @end example
 
-@strong{Warning:} Changing the string by substituting characters does
-change the name of the symbol, but fails to update the obarray, so don't
-do it!
+@strong{Warning:} Never alter the string returned by that function.
+Doing that might make Emacs dysfunctional, and might even crash Emacs.
 @end defun
 
 @cindex uninterned symbol, and generating Lisp code
diff --git a/src/data.c b/src/data.c
index 0f1d881e00b..930d476bc3f 100644
--- a/src/data.c
+++ b/src/data.c
@@ -773,7 +773,10 @@ DEFUN ("symbol-plist", Fsymbol_plist, Ssymbol_plist, 1, 1, 0,
 }
 
 DEFUN ("symbol-name", Fsymbol_name, Ssymbol_name, 1, 1, 0,
-       doc: /* Return SYMBOL's name, a string.  */)
+       doc: /* Return SYMBOL's name, a string.
+
+Warning: never alter the string returned by `symbol-name'.
+Doing that might make Emacs dysfunctional, and might even crash Emacs.  */)
   (register Lisp_Object symbol)
 {
   register Lisp_Object name;
-- 
2.39.0

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Date: Sat, 18 Mar 2023 22:46:35 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: philipk@posteo.net, michael_heerdegen@web.de, mail@daniel-mendler.de, 
>     rpluim@gmail.com, monnier@iro.umontreal.ca, 62009@debbugs.gnu.org, 
>     arstoffel@gmail.com
> 
> > I think being a bit vague here and talking about "dangers" should be 
> > good enough, since not every such code will crash, and probably also not 
> > on every platform and in every build configuration.
> 
> Is the attached patch okay?

Yes, thanks.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Gregory Heytings]

>> Is the attached patch okay?
>
> Yes, thanks.
>

Thanks, pushed (b7f0333355).

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: Philip Kaludercic <philipk@posteo.net>,  michael_heerdegen@web.de,
>   mail@daniel-mendler.de,  monnier@iro.umontreal.ca,  62009@debbugs.gnu.org
> Date: Fri, 10 Mar 2023 09:45:11 +0100
> 
> On Fri, 10 Mar 2023 at 09:11, Eli Zaretskii wrote:
> 
> >> Is it not possible to detect this before the illegal memory access, and
> >> raise a signal in Emacs Lisp?
> >
> > It won't be easy, if at all possible.  And I'm not sure we even want
> > to do that.  What would be the purpose of supporting such a use of
> > Emacs?
> 
> What is the purpose of supporting mutation of symbol names in general?

We don't.  Whoever does this is on their own.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Gregory Heytings]

>>> AFAICT, they _are_ frozen.  These names are in read-only memory, where 
>>> you cannot write.  That's why Emacs crashes, AFAIU: the code is trying 
>>> to write to protected memory.
>>>
>>> Just don't do that, cause it's gonna hurt...
>>
>> Is it not possible to detect this before the illegal memory access, and 
>> raise a signal in Emacs Lisp?
>
> It won't be easy, if at all possible.  And I'm not sure we even want to 
> do that.  What would be the purpose of supporting such a use of Emacs?
>

Instead of raising a signal, I suggest:

diff --git a/src/data.c b/src/data.c
index 0f1d881e00b..76867d6787e 100644
--- a/src/data.c
+++ b/src/data.c
@@ -780,7 +780,7 @@ DEFUN ("symbol-name", Fsymbol_name, Ssymbol_name, 1, 
1, 0,

    CHECK_SYMBOL (symbol);
    name = SYMBOL_NAME (symbol);
-  return name;
+  return build_string (SSDATA (name));
  }

  DEFUN ("bare-symbol", Fbare_symbol, Sbare_symbol, 1, 1, 0,

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

On 3/10/23 10:40, Gregory Heytings wrote:
> Instead of raising a signal, I suggest:
> 
> diff --git a/src/data.c b/src/data.c
> index 0f1d881e00b..76867d6787e 100644
> --- a/src/data.c
> +++ b/src/data.c
> @@ -780,7 +780,7 @@ DEFUN ("symbol-name", Fsymbol_name, Ssymbol_name, 1, 
> 1, 0,
> 
>     CHECK_SYMBOL (symbol);
>     name = SYMBOL_NAME (symbol);
> -  return name;
> +  return build_string (SSDATA (name));
>   }
> 
>   DEFUN ("bare-symbol", Fbare_symbol, Sbare_symbol, 1, 1, 0,

Creating a string is not a good idea since it will lead to an
unacceptably large performance overhead. Raising an exception upon
modification would be the best approach.

Daniel

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Gregory Heytings]

>
> Creating a string is not a good idea since it will lead to an 
> unacceptably large performance overhead.
>

Is "symbol-name" a function that is used in performance-critical code? 
And did you actually measure that performance overhead before concluding 
that it it "unacceptably large"?  According to my measurements, creating a 
string from a symbol name costs about 100 CPU cycles.

>
> Raising an exception upon modification would be the best approach.
>

That would also come with a performance overhead, as there is currently no 
way to distinguist strings that are used for symbol names from other 
strings.  Not to mention the added complexity in the code.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

On 3/10/23 11:59, Gregory Heytings wrote:
> Is "symbol-name" a function that is used in performance-critical code? 
> And did you actually measure that performance overhead before concluding 
> that it it "unacceptably large"?  According to my measurements, creating a 
> string from a symbol name costs about 100 CPU cycles.

Yes, of course, for example completion. It would add cost all over the
place in many packages. Also note the added GC pressure. It is not a
good idea to change symbol-name now to allocate strings.

>> Raising an exception upon modification would be the best approach.
>>
> 
> That would also come with a performance overhead, as there is currently no 
> way to distinguist strings that are used for symbol names from other 
> strings.  Not to mention the added complexity in the code.

One could check if the string is located in read-only memory. Or one
could add a flag bit to the string data structure (and possibly to other
data structures too). Freezing data structures such that they become
read-only is a generally useful feature. There won't be any performance
overhead of the check since a branch not taken is fast thanks to the
branch predictor.

Daniel

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Augusto Stoffel]

On Fri, 10 Mar 2023 at 12:09, Daniel Mendler wrote:

> On 3/10/23 11:59, Gregory Heytings wrote:
>> That would also come with a performance overhead, as there is currently no 
>> way to distinguist strings that are used for symbol names from other 
>> strings.  Not to mention the added complexity in the code.
>
> One could check if the string is located in read-only memory. Or one
> could add a flag bit to the string data structure (and possibly to other
> data structures too). Freezing data structures such that they become
> read-only is a generally useful feature. There won't be any performance
> overhead of the check since a branch not taken is fast thanks to the
> branch predictor.

Note also that in-place modification of strings is arbitrarily costly,
cf. (aset "ascii" 0 ?😼).  Not to mention that it's rarely a good or
necessary move to make, as far as programming style is concerned.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: Gregory Heytings <gregory@heytings.org>,  Eli Zaretskii <eliz@gnu.org>,
>   Philip Kaludercic <philipk@posteo.net>,  michael_heerdegen@web.de,
>   monnier@iro.umontreal.ca,  62009@debbugs.gnu.org
> Date: Fri, 10 Mar 2023 12:23:54 +0100
> 
> On Fri, 10 Mar 2023 at 12:09, Daniel Mendler wrote:
> 
> > On 3/10/23 11:59, Gregory Heytings wrote:
> >> That would also come with a performance overhead, as there is currently no 
> >> way to distinguist strings that are used for symbol names from other 
> >> strings.  Not to mention the added complexity in the code.
> >
> > One could check if the string is located in read-only memory. Or one
> > could add a flag bit to the string data structure (and possibly to other
> > data structures too). Freezing data structures such that they become
> > read-only is a generally useful feature. There won't be any performance
> > overhead of the check since a branch not taken is fast thanks to the
> > branch predictor.
> 
> Note also that in-place modification of strings is arbitrarily costly,
> cf. (aset "ascii" 0 ?😼).  Not to mention that it's rarely a good or
> necessary move to make, as far as programming style is concerned.

Be this tru as it may, we will not constrain what Lisp programs can
legitimately do just because we think it is "rarely a good move".
That's against our long-time policy, which is explain why something
might not be a good idea, but otherwise don't block that, letting the
unwise cope with the consequences of their unwise actions.

IOW, we encourage Lisp programmers to DTRT and not use dangerous
practices, but don't block them if they want to.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Robert Pluim]

>>>>> On Fri, 10 Mar 2023 12:09:59 +0100, Daniel Mendler <mail@daniel-mendler.de> said:

    Daniel> One could check if the string is located in read-only memory. Or one
    Daniel> could add a flag bit to the string data structure (and possibly to other
    Daniel> data structures too). Freezing data structures such that they become
    Daniel> read-only is a generally useful feature. There won't be any performance
    Daniel> overhead of the check since a branch not taken is fast thanks to the
    Daniel> branch predictor.

We already have such a flag:

      /* Number of characters in string; MSB is used as the mark bit.  */
      ptrdiff_t size;
      /* If nonnegative, number of bytes in the string (which is multibyte).
	 If negative, the string is unibyte:
	 -1 for data normally allocated
	 -2 for data in rodata (C string constants)
	 -3 for data that must be immovable (used for bytecode)  */
      ptrdiff_t size_byte;

Try this:

diff --git a/src/lisp.h b/src/lisp.h
index 1276285e2f2..80bbb047824 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1685,6 +1685,8 @@ SREF (Lisp_Object string, ptrdiff_t index)
 INLINE void
 SSET (Lisp_Object string, ptrdiff_t index, unsigned char new)
 {
+  if (XSTRING (string)->u.s.size_byte == -2)
+    Fsignal (Qsetting_constant, string);
   SDATA (string)[index] = new;
 }
 INLINE ptrdiff_t

Robert
-- 

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

On 3/10/23 12:30, Robert Pluim wrote:
>>>>>> On Fri, 10 Mar 2023 12:09:59 +0100, Daniel Mendler <mail@daniel-mendler.de> said:
> 
>     Daniel> One could check if the string is located in read-only memory. Or one
>     Daniel> could add a flag bit to the string data structure (and possibly to other
>     Daniel> data structures too). Freezing data structures such that they become
>     Daniel> read-only is a generally useful feature. There won't be any performance
>     Daniel> overhead of the check since a branch not taken is fast thanks to the
>     Daniel> branch predictor.
> 
> We already have such a flag:
> 
>       /* Number of characters in string; MSB is used as the mark bit.  */
>       ptrdiff_t size;
>       /* If nonnegative, number of bytes in the string (which is multibyte).
> 	 If negative, the string is unibyte:
> 	 -1 for data normally allocated
> 	 -2 for data in rodata (C string constants)
> 	 -3 for data that must be immovable (used for bytecode)  */
>       ptrdiff_t size_byte;

Thanks! That's good. Given that a read only flag already exists, it is
easy to fix the issue. We just have to make sure that the size is
negative for the symbol names and add a check in `aset`.

Daniel

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Date: Fri, 10 Mar 2023 12:36:17 +0100
> Cc: Gregory Heytings <gregory@heytings.org>,
>  Philip Kaludercic <philipk@posteo.net>, michael_heerdegen@web.de,
>  monnier@iro.umontreal.ca, 62009@debbugs.gnu.org, Eli Zaretskii
>  <eliz@gnu.org>, Augusto Stoffel <arstoffel@gmail.com>
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> >       /* Number of characters in string; MSB is used as the mark bit.  */
> >       ptrdiff_t size;
> >       /* If nonnegative, number of bytes in the string (which is multibyte).
> > 	 If negative, the string is unibyte:
> > 	 -1 for data normally allocated
> > 	 -2 for data in rodata (C string constants)
> > 	 -3 for data that must be immovable (used for bytecode)  */
> >       ptrdiff_t size_byte;
> 
> Thanks! That's good. Given that a read only flag already exists, it is
> easy to fix the issue. We just have to make sure that the size is
> negative for the symbol names and add a check in `aset`.

Let's not do that!

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Daniel Mendler]

On 3/10/23 13:13, Eli Zaretskii wrote:
>> Date: Fri, 10 Mar 2023 12:36:17 +0100
>> Cc: Gregory Heytings <gregory@heytings.org>,
>>  Philip Kaludercic <philipk@posteo.net>, michael_heerdegen@web.de,
>>  monnier@iro.umontreal.ca, 62009@debbugs.gnu.org, Eli Zaretskii
>>  <eliz@gnu.org>, Augusto Stoffel <arstoffel@gmail.com>
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>>>       /* Number of characters in string; MSB is used as the mark bit.  */
>>>       ptrdiff_t size;
>>>       /* If nonnegative, number of bytes in the string (which is multibyte).
>>> 	 If negative, the string is unibyte:
>>> 	 -1 for data normally allocated
>>> 	 -2 for data in rodata (C string constants)
>>> 	 -3 for data that must be immovable (used for bytecode)  */
>>>       ptrdiff_t size_byte;
>>
>> Thanks! That's good. Given that a read only flag already exists, it is
>> easy to fix the issue. We just have to make sure that the size is
>> negative for the symbol names and add a check in `aset`.
> 
> Let's not do that!

Why not? There won't be a performance cost.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Dmitry Gutov]

On 10/03/2023 14:24, Daniel Mendler wrote:
> On 3/10/23 13:13, Eli Zaretskii wrote:
>>> Date: Fri, 10 Mar 2023 12:36:17 +0100
>>> Cc: Gregory Heytings<gregory@heytings.org>,
>>>   Philip Kaludercic<philipk@posteo.net>,michael_heerdegen@web.de,
>>>   monnier@iro.umontreal.ca,62009@debbugs.gnu.org, Eli Zaretskii
>>>   <eliz@gnu.org>, Augusto Stoffel<arstoffel@gmail.com>
>>> From: Daniel Mendler<mail@daniel-mendler.de>
>>>
>>>>        /* Number of characters in string; MSB is used as the mark bit.  */
>>>>        ptrdiff_t size;
>>>>        /* If nonnegative, number of bytes in the string (which is multibyte).
>>>> 	 If negative, the string is unibyte:
>>>> 	 -1 for data normally allocated
>>>> 	 -2 for data in rodata (C string constants)
>>>> 	 -3 for data that must be immovable (used for bytecode)  */
>>>>        ptrdiff_t size_byte;
>>> Thanks! That's good. Given that a read only flag already exists, it is
>>> easy to fix the issue. We just have to make sure that the size is
>>> negative for the symbol names and add a check in `aset`.
>> Let's not do that!
> Why not? There won't be a performance cost.

Perhaps we could use some exact benchmark results.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Gregory Heytings]

>
> diff --git a/src/lisp.h b/src/lisp.h
> index 1276285e2f2..80bbb047824 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -1685,6 +1685,8 @@ SREF (Lisp_Object string, ptrdiff_t index)
> INLINE void
> SSET (Lisp_Object string, ptrdiff_t index, unsigned char new)
> {
> +  if (XSTRING (string)->u.s.size_byte == -2)
> +    Fsignal (Qsetting_constant, string);
>   SDATA (string)[index] = new;
> }
> INLINE ptrdiff_t
>

That flag is useful only for the first part of the bug: setting the symbol 
name of a function defined in C.  It does not prevent changing symbol 
names in general, e.g. (aset (symbol-name 'find-file) 1 ?o).

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Gregory Heytings <gregory@heytings.org>,  Philip Kaludercic
>  <philipk@posteo.net>,  michael_heerdegen@web.de,
>   monnier@iro.umontreal.ca,  62009@debbugs.gnu.org,  Eli Zaretskii
>  <eliz@gnu.org>,  Augusto Stoffel <arstoffel@gmail.com>
> Date: Fri, 10 Mar 2023 12:30:48 +0100
> 
> diff --git a/src/lisp.h b/src/lisp.h
> index 1276285e2f2..80bbb047824 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -1685,6 +1685,8 @@ SREF (Lisp_Object string, ptrdiff_t index)
>  INLINE void
>  SSET (Lisp_Object string, ptrdiff_t index, unsigned char new)
>  {
> +  if (XSTRING (string)->u.s.size_byte == -2)
> +    Fsignal (Qsetting_constant, string);

"Setting constant" is misleading.

And again, why do that at all?  It's a waste of cycles, incurred on
_everyone_, for an extremely rare use case that is explicitly
discouraged.  We are not the TSA, and should not adopt their policy of
punishing the innocent 99.99% on behalf of a handful of villains.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Robert Pluim]

>>>>> On Fri, 10 Mar 2023 14:12:25 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: Gregory Heytings <gregory@heytings.org>,  Philip Kaludercic
    >> <philipk@posteo.net>,  michael_heerdegen@web.de,
    >> monnier@iro.umontreal.ca,  62009@debbugs.gnu.org,  Eli Zaretskii
    >> <eliz@gnu.org>,  Augusto Stoffel <arstoffel@gmail.com>
    >> Date: Fri, 10 Mar 2023 12:30:48 +0100
    >> 
    >> diff --git a/src/lisp.h b/src/lisp.h
    >> index 1276285e2f2..80bbb047824 100644
    >> --- a/src/lisp.h
    >> +++ b/src/lisp.h
    >> @@ -1685,6 +1685,8 @@ SREF (Lisp_Object string, ptrdiff_t index)
    >> INLINE void
    >> SSET (Lisp_Object string, ptrdiff_t index, unsigned char new)
    >> {
    >> +  if (XSTRING (string)->u.s.size_byte == -2)
    >> +    Fsignal (Qsetting_constant, string);

    Eli> "Setting constant" is misleading.

True. I was lazy and picked the first one I found.

    Eli> And again, why do that at all?  It's a waste of cycles, incurred on
    Eli> _everyone_, for an extremely rare use case that is explicitly
    Eli> discouraged.  We are not the TSA, and should not adopt their policy of
    Eli> punishing the innocent 99.99% on behalf of a handful of villains.

I wasnʼt seriously proposing it for inclusion, just pointing out that
it was possible.

Robert
-- 

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

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

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Fri, 10 Mar 2023 12:09:59 +0100, Daniel Mendler <mail@daniel-mendler.de> said:
>
>     Daniel> One could check if the string is located in read-only memory. Or one
>     Daniel> could add a flag bit to the string data structure (and possibly to other
>     Daniel> data structures too). Freezing data structures such that they become
>     Daniel> read-only is a generally useful feature. There won't be any performance
>     Daniel> overhead of the check since a branch not taken is fast thanks to the
>     Daniel> branch predictor.
>
> We already have such a flag:
>
>       /* Number of characters in string; MSB is used as the mark bit.  */
>       ptrdiff_t size;
>       /* If nonnegative, number of bytes in the string (which is multibyte).
> 	 If negative, the string is unibyte:
> 	 -1 for data normally allocated
> 	 -2 for data in rodata (C string constants)
> 	 -3 for data that must be immovable (used for bytecode)  */
>       ptrdiff_t size_byte;
>
> Try this:
>
> diff --git a/src/lisp.h b/src/lisp.h
> index 1276285e2f2..80bbb047824 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -1685,6 +1685,8 @@ SREF (Lisp_Object string, ptrdiff_t index)
>  INLINE void
>  SSET (Lisp_Object string, ptrdiff_t index, unsigned char new)
>  {
> +  if (XSTRING (string)->u.s.size_byte == -2)
> +    Fsignal (Qsetting_constant, string);
>    SDATA (string)[index] = new;
>  }
>  INLINE ptrdiff_t
>
> Robert

Why does this have to be in SSET and not in PURE_P?

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Robert Pluim]

>>>>> On Sat, 11 Mar 2023 15:07:56 +0800, Po Lu <luangruo@yahoo.com> said:

    Po Lu> Why does this have to be in SSET and not in PURE_P?

That would work as well, although I thought purespace was going away?

Robert
-- 

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

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

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Sat, 11 Mar 2023 15:07:56 +0800, Po Lu <luangruo@yahoo.com> said:
>
>     Po Lu> Why does this have to be in SSET and not in PURE_P?
>
> That would work as well, although I thought purespace was going away?
>
> Robert

Once pure space goes away, we can change the call to PURE_P in Faset to
only check if the string is read-only.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Daniel Mendler <mail@daniel-mendler.de>,  Philip Kaludercic
>  <philipk@posteo.net>,  michael_heerdegen@web.de,  Gregory Heytings
>  <gregory@heytings.org>,  monnier@iro.umontreal.ca,  62009@debbugs.gnu.org,
>   Eli Zaretskii <eliz@gnu.org>,  Augusto Stoffel <arstoffel@gmail.com>
> Date: Mon, 13 Mar 2023 16:28:10 +0800
> 
> Robert Pluim <rpluim@gmail.com> writes:
> 
> >>>>>> On Sat, 11 Mar 2023 15:07:56 +0800, Po Lu <luangruo@yahoo.com> said:
> >
> >     Po Lu> Why does this have to be in SSET and not in PURE_P?
> >
> > That would work as well, although I thought purespace was going away?
> >
> > Robert
> 
> Once pure space goes away, we can change the call to PURE_P in Faset to
> only check if the string is read-only.

There's no reason to believe someone will remember that.  Just don't
add anything that will require changes when we remove pure space
(except if it's specific to the unexec build).

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: Daniel Mendler <mail@daniel-mendler.de>,  Philip Kaludercic
>>  <philipk@posteo.net>,  michael_heerdegen@web.de,  Gregory Heytings
>>  <gregory@heytings.org>,  monnier@iro.umontreal.ca,  62009@debbugs.gnu.org,
>>   Eli Zaretskii <eliz@gnu.org>,  Augusto Stoffel <arstoffel@gmail.com>
>> Date: Mon, 13 Mar 2023 16:28:10 +0800
>> 
>> Robert Pluim <rpluim@gmail.com> writes:
>> 
>> >>>>>> On Sat, 11 Mar 2023 15:07:56 +0800, Po Lu <luangruo@yahoo.com> said:
>> >
>> >     Po Lu> Why does this have to be in SSET and not in PURE_P?
>> >
>> > That would work as well, although I thought purespace was going away?
>> >
>> > Robert
>> 
>> Once pure space goes away, we can change the call to PURE_P in Faset to
>> only check if the string is read-only.
>
> There's no reason to believe someone will remember that.  Just don't
> add anything that will require changes when we remove pure space
> (except if it's specific to the unexec build).

OK, will keep that in mind.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Daniel Mendler <mail@daniel-mendler.de>,  Philip Kaludercic
>  <philipk@posteo.net>,  michael_heerdegen@web.de,  Gregory Heytings
>  <gregory@heytings.org>,  monnier@iro.umontreal.ca,  62009@debbugs.gnu.org,
>   Eli Zaretskii <eliz@gnu.org>,  Augusto Stoffel <arstoffel@gmail.com>
> Date: Mon, 13 Mar 2023 09:07:20 +0100
> 
> >>>>> On Sat, 11 Mar 2023 15:07:56 +0800, Po Lu <luangruo@yahoo.com> said:
> 
>     Po Lu> Why does this have to be in SSET and not in PURE_P?
> 
> That would work as well, although I thought purespace was going away?

At some point, yes.  So we shouldn't build any new features or
improvements on aspects that will disappear when that happens.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Date: Fri, 10 Mar 2023 10:59:03 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Eli Zaretskii <eliz@gnu.org>, Philip Kaludercic <philipk@posteo.net>, 
>     michael_heerdegen@web.de, monnier@iro.umontreal.ca, 62009@debbugs.gnu.org, 
>     Augusto Stoffel <arstoffel@gmail.com>
> 
> > Creating a string is not a good idea since it will lead to an 
> > unacceptably large performance overhead.
> 
> Is "symbol-name" a function that is used in performance-critical code? 

Yes, it is.  Just grep for it.  We even call it from C quite a few
times.  And processing is just one aspect of that; memory and GC is
another, not less important.

> And did you actually measure that performance overhead before concluding 
> that it it "unacceptably large"?

Anything greater than zero is unacceptably large from where I stand,
when the other side of the balance is the use case against which this
protects.

> > Raising an exception upon modification would be the best approach.
> 
> That would also come with a performance overhead, as there is currently no 
> way to distinguist strings that are used for symbol names from other 
> strings.  Not to mention the added complexity in the code.

Which is why we should do neither.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> Date: Fri, 10 Mar 2023 09:40:31 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Philip Kaludercic <philipk@posteo.net>, michael_heerdegen@web.de, 
>     mail@daniel-mendler.de, monnier@iro.umontreal.ca, 62009@debbugs.gnu.org
> 
> 
> Instead of raising a signal, I suggest:
> 
> diff --git a/src/data.c b/src/data.c
> index 0f1d881e00b..76867d6787e 100644
> --- a/src/data.c
> +++ b/src/data.c
> @@ -780,7 +780,7 @@ DEFUN ("symbol-name", Fsymbol_name, Ssymbol_name, 1, 
> 1, 0,
> 
>     CHECK_SYMBOL (symbol);
>     name = SYMBOL_NAME (symbol);
> -  return name;
> +  return build_string (SSDATA (name));
>   }
> 
>   DEFUN ("bare-symbol", Fbare_symbol, Sbare_symbol, 1, 1, 0,

No, we will NOT increase GC pressure in Emacs just because someone
could do a silly and nonsensical thing.  No way.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Gregory Heytings]

>
> No, we will NOT increase GC pressure in Emacs just because someone could 
> do a silly and nonsensical thing.  No way.
>

I agree with you that this is not really a bug, so doing nothing is fine 
for me, too.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 10 Mar 2023 09:40:31 +0000
>> From: Gregory Heytings <gregory@heytings.org>
>> cc: Philip Kaludercic <philipk@posteo.net>, michael_heerdegen@web.de, 
>>     mail@daniel-mendler.de, monnier@iro.umontreal.ca, 62009@debbugs.gnu.org
>> 
>> 
>> Instead of raising a signal, I suggest:
>> 
>> diff --git a/src/data.c b/src/data.c
>> index 0f1d881e00b..76867d6787e 100644
>> --- a/src/data.c
>> +++ b/src/data.c
>> @@ -780,7 +780,7 @@ DEFUN ("symbol-name", Fsymbol_name, Ssymbol_name, 1, 
>> 1, 0,
>> 
>>     CHECK_SYMBOL (symbol);
>>     name = SYMBOL_NAME (symbol);
>> -  return name;
>> +  return build_string (SSDATA (name));
>>   }
>> 
>>   DEFUN ("bare-symbol", Fbare_symbol, Sbare_symbol, 1, 1, 0,
>
> No, we will NOT increase GC pressure in Emacs just because someone
> could do a silly and nonsensical thing.  No way.

Can't we make puresize.h check (in addition to whether or not the string
is in pure space) whether or not the string lies in read-only segments
of the executable?

Or maybe put all of string data of DEFSYM'd symbols in pure space, since
Faset already checks that the string is not in pure space.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Gregory Heytings <gregory@heytings.org>,  michael_heerdegen@web.de,
>   mail@daniel-mendler.de,  philipk@posteo.net,  monnier@iro.umontreal.ca,
>   62009@debbugs.gnu.org
> Date: Sat, 11 Mar 2023 15:07:05 +0800
> 
> > No, we will NOT increase GC pressure in Emacs just because someone
> > could do a silly and nonsensical thing.  No way.
> 
> Can't we make puresize.h check (in addition to whether or not the string
> is in pure space) whether or not the string lies in read-only segments
> of the executable?
> 
> Or maybe put all of string data of DEFSYM'd symbols in pure space, since
> Faset already checks that the string is not in pure space.

Let me remind us all that we intend to toss pure space soon.  So let's
not build any new features on what pure space means and does,
certainly not for such marginal use cases.  We don't want to take upon
ourselves any jobs that might prove difficult to keep doing when some
of the underlying infrastructure changes, because it would be
ridiculous to have this little tail wag the Emacs dog when the time
comes.

bug#62009: 29.0.60; Emacs crashes on setf symbol-name

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Daniel Mendler <mail@daniel-mendler.de>,  michael_heerdegen@web.de,
>>   monnier@iro.umontreal.ca,  62009@debbugs.gnu.org
>> Date: Thu, 09 Mar 2023 21:11:13 +0000
>> 
>> > AFAICT, they _are_ frozen.  These names are in read-only memory, where
>> > you cannot write.  That's why Emacs crashes, AFAIU: the code is trying
>> > to write to protected memory.
>> >
>> > Just don't do that, cause it's gonna hurt...
>> 
>> Is it not possible to detect this before the illegal memory access, and
>> raise a signal in Emacs Lisp?
>
> It won't be easy, if at all possible.  And I'm not sure we even want
> to do that.  What would be the purpose of supporting such a use of
> Emacs?

The point is not that much that a signal is raised, but that anything
else but segfaulting is done.

Emacs is generally not "safe", but the idea of someone stuffing an
expression like the one mentioned in this bug report into the autoload
of a package just to cause chaos is uncompfortable.  Of course, you can
already do

    ;;;###autoload (delete-directory "~" t)

or

    ;;;###autoload (fset 'eval 'ignore)

so maybe my point is moot.

-- 
Philip Kaludercic