bug#70182: [PATCH] Fix + ert for segmentation fault in get-variable-watchers (1 of 9)

bug#70182: [PATCH] Fix + ert for segmentation fault in get-variable-watchers (1 of 9)

[Robert Burks]

[-- Attachment #1.1: Type: text/plain, Size: 3849 bytes --]

(1 of 9)
** All patches in this and following emails have been re-based and tested on
   7a41de3d515 04/03/2024 **

The following segmentation faults are repeatable on version 27.1 that is
obtained through debian apt as well the current 30.0.50 master
branch (I pull and rebuild regularly).

I have attached a patch which includes a fix and ert.
(Please note the BUG# placeholder in six(6) places will need to be updated.)
I have rebased on master then rebuilt and tested the results.
The function that caused the problem had no tests previously.

The c DEFUN get_variable_watchers used the function SYMBOL_TRAPPED_WRITE_P
without guarding its input.  This inline function is an interface to
the XSYMBOL function for accessing symbol structure data and has no
provision for type checking its inputs.  XSYMBOL changed between
27.1 and 30.0.50 (2128cd8c08d). So while they will both crash with numbers
or lists a string or a variable with a string value will not crash the later
version, though it wrongly returns nil.  However, the XSYMBOL function is
not
the cause of the issue, it merely concealed the issue in some situations.
The
patch included fixes the issue in get_variable_watchers going back much
further.
***Update, builds from the end of January on now also seg fault.

The other functions in the variable-watcher family do not suffer the
same flaw as they catch invalid inputs with CHECK_SYMBOL or rely on the
error checking in Fget.

Also, while simply putting CHECK_SYMBOL at the top would have solved this
one
problem, there is a reasoning behind me moving Findirect_variable to the
top as
well. One, is to make it more like the other functions in it's family.
Second, is
that it is actually required to fix other bugs that I discovered and will
layout
over the course of several bug reports, associated patches, and ert's.

Bug Recreation Follows------------------------------------------------

WARNING: This crash requires just the 1 command below.
         Do not repeat in an Emacs session you are not
         willing to lose immediately. (run under gdb control ideally)

Session 1-------------------------------------------------------------

emacs -Q
(get-variable-watchers load-path) ;; opps forgot the apostrophe....

Fatal error 11: Segmentation fault

Session 2-------------------------------------------------------------
Update *****These Bugs Now All Crash in 30.0.50 after the end of
January******

emacs -Q
(get-variable-watchers nil)
nil
(defvar test-str "test")
test-str
(get-variable-watchers test-str)    ;; no apostrophe, value = string
nil                                 ;; no error?, no crash at least (this
crashes 27.1)
(get-variable-watchers 'test-str)
nil                                 ;; expected result
(get-variable-watchers "test")
nil                                 ;; no error?, no crash (this crashes
27.1)
(defvar test-num 5)                 ;; let's try a number with watcher
test-num
(add-variable-watcher 'test-num (lambda () nil))
nil
(get-variable-watchers 'test-num)
((closure (t) nil nil))             ;; expected result
(get-variable-watchers "test-num")  ;; string = symbol name
nil                                 ;; no error?, no crash (this crashes
27.1)
(get-variable-watchers test-num)    ;; no apostrophe, value = number
---seg fault---                     ;;crash

Session 3-------------------------------------------------------------

;; A number as parameter also crashes.
;; It also happens in batch mode.

>emacs --batch --eval "(get-variable-watchers 5)"
Fatal error 11: Segmentation fault

Other Crashes --------------------------------------------------------

;; Session 1 was a symbol with a list of strings as it's value.
;; So I also tried some direct lists.

(get-variable-watchers '(1 2 3))
---seg fault---

(get-variable-watchers '("test" "test"))
---seg fault---

[-- Attachment #1.2: Type: text/html, Size: 4347 bytes --]

[-- Attachment #2: 0001-Fix-ert-segmentation-fault-in-get-variable-watchers-.patch --]
[-- Type: application/x-patch, Size: 3447 bytes --]

bug#70182: [PATCH] Fix + ert for segmentation fault in get-variable-watchers (1 of 9)

[Eli Zaretskii]

> From: Robert Burks <rburksdev@gmail.com>
> Date: Thu, 4 Apr 2024 04:44:22 -0400
> 
> +++ b/src/data.c
> @@ -1838,7 +1838,7 @@ SYMBOL (or its aliases) are set.  */)
>    (Lisp_Object symbol, Lisp_Object watch_function)
>  {
>    symbol = Findirect_variable (symbol);
> -  Lisp_Object watchers = Fget (symbol, Qwatchers);
> +  Lisp_Object watchers = Fget (symbol, Qwatchers); /* CHECK_SYMBOL is in Fget */

This commentary is not needed here, so please remove it.

Also, please note that our conventions are to end a comment in a
period and two spaces.

> +  symbol = Findirect_variable (symbol);
> +  CHECK_SYMBOL (symbol); /* BUG#00000 Must guard SYMBOL_TRAPPED_WRITE_P */

This comment is also redundant: the fact that we check a symbol
doesn't need any explanations.

Thanks.

bug#70182: [PATCH] Fix + ert for segmentation fault in get-variable-watchers (1 of 9)

[Eli Zaretskii]

[Please use Reply All to reply, to keep the bug tracker CC'ed.]

> From: Robert Burks <rburksdev@gmail.com>
> Date: Sun, 7 Apr 2024 02:30:54 -0400
> 
> I will make the changes.  Sorry if my previous reply came across as rash, wasn't the intent.
> Should I remove the bug number tag also?

The bug number should be mentioned in the commit log message, not in
the sources.

> Also, kicking myself, I know they should end with a period and two spaces, every other
> comment I've written probably does.  Somehow the only ones I didn't check was the very
> first patch, lol.  That first comment wasn't even supposed to go out, that was my personal note
> for when I was going through looking for more of the same bug.
> 
> There will probably be other comments to remove.

Thanks.

bug#70182: [PATCH] Fix + ert for segmentation fault in get-variable-watchers (1 of 9)

[Robert Burks]

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

Got it.

On Sun, Apr 7, 2024 at 3:58 AM Eli Zaretskii <eliz@gnu.org> wrote:

> [Please use Reply All to reply, to keep the bug tracker CC'ed.]
>
> > From: Robert Burks <rburksdev@gmail.com>
> > Date: Sun, 7 Apr 2024 02:30:54 -0400
> >
> > I will make the changes.  Sorry if my previous reply came across as
> rash, wasn't the intent.
> > Should I remove the bug number tag also?
>
> The bug number should be mentioned in the commit log message, not in
> the sources.
>
> > Also, kicking myself, I know they should end with a period and two
> spaces, every other
> > comment I've written probably does.  Somehow the only ones I didn't
> check was the very
> > first patch, lol.  That first comment wasn't even supposed to go out,
> that was my personal note
> > for when I was going through looking for more of the same bug.
> >
> > There will probably be other comments to remove.
>
> Thanks.
>

[-- Attachment #2: Type: text/html, Size: 1409 bytes --]

bug#70182: [PATCH] Fix + ert for segmentation fault in get-variable-watchers (1 of 9)

[Stefan Kangas]

Robert Burks <rburksdev@gmail.com> writes:

> Got it.
>
> On Sun, Apr 7, 2024 at 3:58 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
>> [Please use Reply All to reply, to keep the bug tracker CC'ed.]
>>
>> > From: Robert Burks <rburksdev@gmail.com>
>> > Date: Sun, 7 Apr 2024 02:30:54 -0400
>> >
>> > I will make the changes.  Sorry if my previous reply came across as
>> rash, wasn't the intent.
>> > Should I remove the bug number tag also?
>>
>> The bug number should be mentioned in the commit log message, not in
>> the sources.
>>
>> > Also, kicking myself, I know they should end with a period and two
>> spaces, every other
>> > comment I've written probably does.  Somehow the only ones I didn't
>> check was the very
>> > first patch, lol.  That first comment wasn't even supposed to go out,
>> that was my personal note
>> > for when I was going through looking for more of the same bug.
>> >
>> > There will probably be other comments to remove.
>>
>> Thanks.
>>

Any news here?

The patch looks okay to me, so perhaps we should just fix up the minor
stylistic issues and install it?