bug#70185: [PATCH] Fix + ert for numerous bugs involving re-aliasing, uninterned symbols, and watchers (4 of 9)

bug#70185: [PATCH] Fix + ert for numerous bugs involving re-aliasing, uninterned symbols, and watchers (4 of 9)

[Robert Burks]

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

(4 of 9)

Bug#00003a (Bypass watcher by adding an uninterned symbol as an alias
before watcher.)
Bug#00003b (re-aliasing the middle of a chain to a constant)
Bug#00003c (re-aliasing the middle of a chain from an unwatched to a watched
            variable then writing from the end bypasses watching.)
Bug#00003d (re-aliasing notifies the base variable watcher that it is
becoming an alias)

** Bug recreations are at the end

My patches for the three bugs prior essentially fix these bugs.  The
included patches are basically some clean up work and closing gaps when
working
with constants.  That is, uninterned symbols would have worked as a side
effect
of the previous submissions.  But some cleanup, flag management, and some
small
fixes for constants were left over.

The last two bugs happen because 'trapped_write' is only "harmonized" across
the obarray in the event of watcher adding and removing and not in the event
of re-aliasing.  I will show in testing that I was able to close up these
bugs
and also remove the need for "harmonizing".  My previous patches fix the
setting of the constant and the included patches enforce re-aliasing
behavior.

Oddly, existing testing calls 'defvar' on the alias prior to 'defvaralias'
to
seemingly side step the fact that they do not work with uninterned symbols.
Maybe things have changed?  As near as I can tell, the evaluation process
(such as 'eval-sexp-add-defvars') seems to see that 'defvaralias' starts
with
'def' and interns its arguments as part of evaluation.  For now I was able
to
feed 'defvaralias' uninterned symbols using 'gensym' and 'make-symbol'.

Should uninterned symbols be allowed to be a variable alias?  Currently it
is
allowed and could be easily prevented in 'defvaralias'.  Currently, the base
variable being uninterned is also allowed, though as long as it is the only
one
and last in the chain the current system will work.  Regardless of
uninterned
symbols being allowed the included patches apply as they remove the need to
"harmonize" even for interned symbols (this "harmonizing" wasn't even
occurring
at all required times anyways).

In regards to bug '3d' from above, if an alias of a watched variable gets
re-aliased there should be no notification, the warning of losing value is
all
that should happen if the value is different.  Currently, notification not
only
happens, it happens wrong (see example below).

Consider a variable with a watcher function designed only to watch it.
Then an
alias is added and then re-aliased, suddenly the watcher function would
receive
notifications about symbols other than that which it was designed for.  A
watcher function should be able to assume that any notification if receives
is
for the operation applying to the base variable of any given alias chain.  A
base variable that has watchers and then becomes an alias is no longer a
base
variable.  The only time a watcher should receive notifications for multiple
symbols is if it is explicitly assigned to multiple variables/chains.  That
is a
watcher function shouldn't gain additional symbols names being sent to it
from a
'defvaralias' assignment happening somewhere along its alias chain.  This
allows
one to write simple watchers for the general case without the need to
anticipate
future outside actions.

Additionally, once a variable becomes an alias if it was watched those
watchers
should be removed (after notification that it is about to become an alias,
which
is questionable as to its actual value as a feature).  We shouldn't save the
watchers to fallback on in the event of re-aliasing as once a variable
becomes
an alias those watchers were made inaccessible (i.e. the user cannot add or
remove watchers as those functions will follow the redirection).

Notification should always be based on the 'base' variable only.  When a
watcher is
assigned to an alias the assignment follows redirection and is applied to
the
base variable.  Notifications when working properly use the base variable
symbol
name (this is existing behaviour and will continue).  Ideally if one adds
a watcher to a variable that will rely on the name of the symbol it should
use
'Findirect_variable' on said variable to discover if it points elsewhere.
Also, I have a missing feature(Fvaralias-p) ready to add after these nine
emails.

----------------------------------------------------------------------------

Patch 0006: These functions were the only ones that checked for constant
after the redirection loop, however, they were still using the original
variable.
I moved it to the top to make all these functions look alike and made them
checking for constants along every step of redirection.  Additionally, it
eliminates an extra conversion that was occurring in the most common case.

Patch 0007: makunbound was making an unneeded check for constant, it already
happens after Fset called set_internal.  Additionally, this check didn't
follow redirection and the one in Fset will.  Also, the very first thing
set_internal does is call CHECK_SYMBOL.  I eliminated the extra function
call,
call set_internal directly and let it do all the work.

Patch 0008: Eliminated the usage of an enumeration as a boolean.

Patch 0009: Eliminated the usage of an enumeration as a boolean.
Additionally,
the commentary had no mention of thread switching. This section of code and
commentary mostly predates the addition of "SET_INTERNAL_THREAD_SWITCH" to
the
enumeration.  Which points out a downside of using an enumeration as a
boolean,
someone can add to it or rearrange it and overlook its boolean usages.

Patch 0010: Added commentary regarding input sanitation. Findirect_variable
converts a Lisp_Object to Lisp_Symbol and then back to a Lisp_Object even
when
there is no alias redirection.  I almost added brackets to make it go right
through when there is no alias.  However, I realized it had the added
benefit
that a calling function can use it to make sure their parameter is a bare
symbol.

Patch 0011: With all previous changes only 'TRAPPED_NOWRITE' will be
copied to an alias that is explicitly aliased to a constant (or to another
alias that was explicitly aliased to a constant).  Other functions check
for constant
as they follow redirection all the way to the end.  An alias set as constant
cannot be re-aliased.  I added checking in 'defvaralias' that will prevent
any
alias in the upstream of an alias that got re-aliased to a constant from
being
re-aliased itself (maybe we can allow them to be re-aliased, since they were
not explicitly set? Though until then they are still perceived as constant
to
everything including 'defvaralias'). Because of previous bug fixes the other
values of the flag along the chain are essentially ignored, so this is
really
just making the flag have a consistent value (i.e. SYMBOL_TRAPPED_WRITE only
applies to non-aliases) and fixing gaps in re-aliasing.

Patch 0012: This applies on top of the previous and is just to get
rid of a needless conversion call.

Patch 0013: Harmonizing is no longer required at all due to previous bug
fixes.
My testing attached along with all other regression tests passing proves
this.

Patch 0014: This is a full test showing that aliases and watchers now
work with uninterned symbols (do we allow them is secondary). Using them
allowed this test of aliases to not clutter up the global symbol space
and have the test simply be garbage collected away (it's also repeatable
interactively).  Now that they work, maybe someone (me lol, I have testing
applications for aliases themselves that could use this functionality) will
find them to be a useful tool.  But at least now they cannot be accidentally
used and circumvent a watcher.
(Please note the BUG# placeholder in three(3) places will need to be
updated.)

Patch 0015: Test for the specific chain breaking and re-aliasing bugs
below. I placed this in data-tests.el because it involves watchers.
(Please note the BUG# placeholder in seven(7) places will need to be
updated.)

Interestingly enough the uninterned symbols test actually stems from the
very
first test I attempted to write for the first bug.  I often use uninterned
symbols while testing as they make it easy to make tests that can be
repeated
without mucking up the symbol space.


Bug
Recreation-------------------------------------------------------------------

This occurs because adding an uninterned alias before watchers are added
makes them never get their 'trapped_write' flag set as they are never
"harmonized".

Bug#00003a (Bypass watcher by adding an uninterned symbol as alias before
watcher)
emacs
-Q-------------------------------------------------------------------------
(defvar results nil)
results

(defvar fake (gensym))
fake

(defvar notsafe "I should be watched")
notsafe

(defvaralias fake 'notsafe)
notsafe

(add-variable-watcher 'notsafe (lambda (&rest args) (push args results)))
nil

(set fake "bypassed")
"bypassed"

notsafe
"bypassed"

results
nil
;; There should be watch results

Bug#00003a (Bypass watcher by adding an uninterned symbol as alias before
watcher)
emacs
-Q-------------------------------------------------------------------------
(defvar results nil)
results

(defvar fake (make-symbol "dummy"))
fake

(defvar notsafe "I should be watched")
notsafe

(defvaralias fake 'notsafe)
notsafe

(add-variable-watcher 'notsafe (lambda (&rest args) (push args results)))
nil

(set fake "bypassed")
"bypassed"

notsafe
"bypassed"

results
nil


Bug#00003b (re-aliasing the base/middle of a chain to a constant)
emacs -
Q------------------------------------------------------------------------

(defvar test 5)
test

(defvaralias 'testa1 'test)
test

(defvaralias 'testa2 'testa1)
testa1

(defvaralias 'testa1 'nil)
nil

(set 'testa2 5)  ;; makunbound works also
5

nil
5
;; We just set a constant

Bug#00003c (re-aliasing the middle of a chain from an unwatched to a watched
            variable then writing from the end bypasses watching.)
emacs
-Q-------------------------------------------------------------------------

(defvar results nil)
results

;; watched
(defvar test 5)
test

;; unwatched
(defvar test2 100)
test2

(add-variable-watcher 'test (lambda (&rest args) (push args results)))
nil

(defvaralias 'testa1 'test2)
test2

(defvaralias 'testa2 'testa1)
testa1

(defvaralias 'testa1 'test)
test

(set 'testa2 500)
500

test
500

results
nil
;; There should be watch results

Bug#00003d (re-aliasing notifies the base variable watcher that it is
becoming an alias)
emacs
-Q-------------------------------------------------------------------------
(defvar results nil)
results

(defvar test 5)
test

(defvar test2 10)
test2

(add-variable-watcher 'test (lambda (&rest args) (push args results)))
nil

(defvaralias 'testa 'test)
test

(defvaralias 'testa 'test2)
test2

results
((test test2 defvaralias nil))

test
5

testa
10

;; 'test' is not becoming an alias of 'test2', 'testa' was re-aliased to
'test2'.
;; No notification should be made in this instance, the warning is
sufficient.

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

[-- Attachment #2: 0014-Add-testing-for-variable-watchers-using-uninterned-s.patch --]
[-- Type: application/x-patch, Size: 4099 bytes --]

[-- Attachment #3: 0012-src-eval.c-defvaralias-Removed-extra-inline-conversi.patch --]
[-- Type: application/x-patch, Size: 1013 bytes --]

[-- Attachment #4: 0011-Changed-trapped_write-flag-mechanics-and-added-exten.patch --]
[-- Type: application/x-patch, Size: 4721 bytes --]

[-- Attachment #5: 0015-Added-ert-for-variable-alias-chain-and-re-aliasing-b.patch --]
[-- Type: application/x-patch, Size: 3734 bytes --]

[-- Attachment #6: 0013-trapped_write-no-longer-needs-to-be-harmonized-acros.patch --]
[-- Type: application/x-patch, Size: 2379 bytes --]

[-- Attachment #7: 0008-src-eval.c-do_specbind-Avoid-using-enumeration-as-bo.patch --]
[-- Type: application/x-patch, Size: 796 bytes --]

[-- Attachment #8: 0010-Added-commentary.patch --]
[-- Type: application/x-patch, Size: 1581 bytes --]

[-- Attachment #9: 0009-src-data.c-set_internal-Avoid-using-enumeration-as-b.patch --]
[-- Type: application/x-patch, Size: 1094 bytes --]

[-- Attachment #10: 0007-src-data.c-Fmakunbound-Now-calls-set_internal-direct.patch --]
[-- Type: application/x-patch, Size: 1011 bytes --]

[-- Attachment #11: 0006-Make-checking-for-constant-occur-along-redirection.patch --]
[-- Type: application/x-patch, Size: 2190 bytes --]

bug#70185: [PATCH] Fix + ert for numerous bugs involving re-aliasing, uninterned symbols, and watchers (4 of 9)

[Eli Zaretskii]

> From: Robert Burks <rburksdev@gmail.com>
> Date: Thu, 4 Apr 2024 04:45:40 -0400
> 
> (4 of 9)
> 
> Bug#00003a (Bypass watcher by adding an uninterned symbol as an alias before watcher.)
> Bug#00003b (re-aliasing the middle of a chain to a constant)
> Bug#00003c (re-aliasing the middle of a chain from an unwatched to a watched
>             variable then writing from the end bypasses watching.)
> Bug#00003d (re-aliasing notifies the base variable watcher that it is becoming an alias)
> 
> ** Bug recreations are at the end
> 
> My patches for the three bugs prior essentially fix these bugs.  The
> included patches are basically some clean up work and closing gaps when working
> with constants.  That is, uninterned symbols would have worked as a side effect
> of the previous submissions.  But some cleanup, flag management, and some small
> fixes for constants were left over.
> 
> The last two bugs happen because 'trapped_write' is only "harmonized" across
> the obarray in the event of watcher adding and removing and not in the event
> of re-aliasing.  I will show in testing that I was able to close up these bugs
> and also remove the need for "harmonizing".  My previous patches fix the 
> setting of the constant and the included patches enforce re-aliasing behavior.
> 
> Oddly, existing testing calls 'defvar' on the alias prior to 'defvaralias' to
> seemingly side step the fact that they do not work with uninterned symbols.
> Maybe things have changed?  As near as I can tell, the evaluation process
> (such as 'eval-sexp-add-defvars') seems to see that 'defvaralias' starts with
> 'def' and interns its arguments as part of evaluation.  For now I was able to
> feed 'defvaralias' uninterned symbols using 'gensym' and 'make-symbol'.
> 
> Should uninterned symbols be allowed to be a variable alias?  Currently it is
> allowed and could be easily prevented in 'defvaralias'.  Currently, the base
> variable being uninterned is also allowed, though as long as it is the only one
> and last in the chain the current system will work.  Regardless of uninterned
> symbols being allowed the included patches apply as they remove the need to
> "harmonize" even for interned symbols (this "harmonizing" wasn't even occurring
> at all required times anyways).
> 
> In regards to bug '3d' from above, if an alias of a watched variable gets
> re-aliased there should be no notification, the warning of losing value is all
> that should happen if the value is different.  Currently, notification not only
> happens, it happens wrong (see example below).
> 
> Consider a variable with a watcher function designed only to watch it.  Then an
> alias is added and then re-aliased, suddenly the watcher function would receive
> notifications about symbols other than that which it was designed for.  A
> watcher function should be able to assume that any notification if receives is
> for the operation applying to the base variable of any given alias chain.  A
> base variable that has watchers and then becomes an alias is no longer a base
> variable.  The only time a watcher should receive notifications for multiple
> symbols is if it is explicitly assigned to multiple variables/chains.  That is a
> watcher function shouldn't gain additional symbols names being sent to it from a
> 'defvaralias' assignment happening somewhere along its alias chain.  This allows
> one to write simple watchers for the general case without the need to anticipate
> future outside actions.
> 
> Additionally, once a variable becomes an alias if it was watched those watchers
> should be removed (after notification that it is about to become an alias, which
> is questionable as to its actual value as a feature).  We shouldn't save the
> watchers to fallback on in the event of re-aliasing as once a variable becomes
> an alias those watchers were made inaccessible (i.e. the user cannot add or
> remove watchers as those functions will follow the redirection).
> 
> Notification should always be based on the 'base' variable only.  When a watcher is
> assigned to an alias the assignment follows redirection and is applied to the
> base variable.  Notifications when working properly use the base variable symbol
> name (this is existing behaviour and will continue).  Ideally if one adds
> a watcher to a variable that will rely on the name of the symbol it should use
> 'Findirect_variable' on said variable to discover if it points elsewhere.
> Also, I have a missing feature(Fvaralias-p) ready to add after these nine emails.
> 
> ----------------------------------------------------------------------------
> 
> Patch 0006: These functions were the only ones that checked for constant
> after the redirection loop, however, they were still using the original variable.
> I moved it to the top to make all these functions look alike and made them
> checking for constants along every step of redirection.  Additionally, it
> eliminates an extra conversion that was occurring in the most common case.
> 
> Patch 0007: makunbound was making an unneeded check for constant, it already
> happens after Fset called set_internal.  Additionally, this check didn't
> follow redirection and the one in Fset will.  Also, the very first thing
> set_internal does is call CHECK_SYMBOL.  I eliminated the extra function call,
> call set_internal directly and let it do all the work.
> 
> Patch 0008: Eliminated the usage of an enumeration as a boolean.
> 
> Patch 0009: Eliminated the usage of an enumeration as a boolean. Additionally,
> the commentary had no mention of thread switching. This section of code and
> commentary mostly predates the addition of "SET_INTERNAL_THREAD_SWITCH" to the
> enumeration.  Which points out a downside of using an enumeration as a boolean,
> someone can add to it or rearrange it and overlook its boolean usages.
> 
> Patch 0010: Added commentary regarding input sanitation. Findirect_variable
> converts a Lisp_Object to Lisp_Symbol and then back to a Lisp_Object even when
> there is no alias redirection.  I almost added brackets to make it go right
> through when there is no alias.  However, I realized it had the added benefit
> that a calling function can use it to make sure their parameter is a bare symbol.
> 
> Patch 0011: With all previous changes only 'TRAPPED_NOWRITE' will be
> copied to an alias that is explicitly aliased to a constant (or to another
> alias that was explicitly aliased to a constant).  Other functions check for constant
> as they follow redirection all the way to the end.  An alias set as constant
> cannot be re-aliased.  I added checking in 'defvaralias' that will prevent any
> alias in the upstream of an alias that got re-aliased to a constant from being
> re-aliased itself (maybe we can allow them to be re-aliased, since they were
> not explicitly set? Though until then they are still perceived as constant to
> everything including 'defvaralias'). Because of previous bug fixes the other
> values of the flag along the chain are essentially ignored, so this is really
> just making the flag have a consistent value (i.e. SYMBOL_TRAPPED_WRITE only
> applies to non-aliases) and fixing gaps in re-aliasing. 
> 
> Patch 0012: This applies on top of the previous and is just to get
> rid of a needless conversion call.
> 
> Patch 0013: Harmonizing is no longer required at all due to previous bug fixes.
> My testing attached along with all other regression tests passing proves this.
> 
> Patch 0014: This is a full test showing that aliases and watchers now
> work with uninterned symbols (do we allow them is secondary). Using them
> allowed this test of aliases to not clutter up the global symbol space
> and have the test simply be garbage collected away (it's also repeatable
> interactively).  Now that they work, maybe someone (me lol, I have testing
> applications for aliases themselves that could use this functionality) will
> find them to be a useful tool.  But at least now they cannot be accidentally
> used and circumvent a watcher.
> (Please note the BUG# placeholder in three(3) places will need to be updated.)
> 
> Patch 0015: Test for the specific chain breaking and re-aliasing bugs
> below. I placed this in data-tests.el because it involves watchers.
> (Please note the BUG# placeholder in seven(7) places will need to be updated.)
> 
> Interestingly enough the uninterned symbols test actually stems from the very
> first test I attempted to write for the first bug.  I often use uninterned
> symbols while testing as they make it easy to make tests that can be repeated
> without mucking up the symbol space.
> 
> Bug Recreation-------------------------------------------------------------------
> 
> This occurs because adding an uninterned alias before watchers are added
> makes them never get their 'trapped_write' flag set as they are never
> "harmonized".
> 
> Bug#00003a (Bypass watcher by adding an uninterned symbol as alias before watcher)
> emacs -Q-------------------------------------------------------------------------
> (defvar results nil)
> results
> 
> (defvar fake (gensym))
> fake
> 
> (defvar notsafe "I should be watched")
> notsafe
> 
> (defvaralias fake 'notsafe)
> notsafe
> 
> (add-variable-watcher 'notsafe (lambda (&rest args) (push args results)))
> nil
> 
> (set fake "bypassed")
> "bypassed"
> 
> notsafe
> "bypassed"
> 
> results
> nil
> ;; There should be watch results
> 
> Bug#00003a (Bypass watcher by adding an uninterned symbol as alias before watcher)
> emacs -Q-------------------------------------------------------------------------
> (defvar results nil)
> results
> 
> (defvar fake (make-symbol "dummy"))
> fake
> 
> (defvar notsafe "I should be watched")
> notsafe
> 
> (defvaralias fake 'notsafe)
> notsafe
> 
> (add-variable-watcher 'notsafe (lambda (&rest args) (push args results)))
> nil
> 
> (set fake "bypassed")
> "bypassed"
> 
> notsafe
> "bypassed"
> 
> results
> nil
> 
> Bug#00003b (re-aliasing the base/middle of a chain to a constant)
> emacs - Q------------------------------------------------------------------------
> 
> (defvar test 5)
> test
> 
> (defvaralias 'testa1 'test)
> test
> 
> (defvaralias 'testa2 'testa1)
> testa1
> 
> (defvaralias 'testa1 'nil)
> nil
> 
> (set 'testa2 5)  ;; makunbound works also
> 5
> 
> nil
> 5
> ;; We just set a constant
> 
> Bug#00003c (re-aliasing the middle of a chain from an unwatched to a watched
>             variable then writing from the end bypasses watching.)
> emacs -Q-------------------------------------------------------------------------
> 
> (defvar results nil)
> results
> 
> ;; watched
> (defvar test 5)
> test
> 
> ;; unwatched
> (defvar test2 100)
> test2
> 
> (add-variable-watcher 'test (lambda (&rest args) (push args results)))
> nil
> 
> (defvaralias 'testa1 'test2)
> test2
> 
> (defvaralias 'testa2 'testa1)
> testa1
> 
> (defvaralias 'testa1 'test)
> test
> 
> (set 'testa2 500)
> 500
> 
> test
> 500
> 
> results
> nil
> ;; There should be watch results
> 
> Bug#00003d (re-aliasing notifies the base variable watcher that it is becoming an alias)
> emacs -Q-------------------------------------------------------------------------
> (defvar results nil)
> results
> 
> (defvar test 5)
> test
> 
> (defvar test2 10)
> test2
> 
> (add-variable-watcher 'test (lambda (&rest args) (push args results)))
> nil
> 
> (defvaralias 'testa 'test)
> test
> 
> (defvaralias 'testa 'test2)
> test2
> 
> results
> ((test test2 defvaralias nil))
> 
> test
> 5
> 
> testa
> 10
> 
> ;; 'test' is not becoming an alias of 'test2', 'testa' was re-aliased to 'test2'.
> ;; No notification should be made in this instance, the warning is sufficient.

It sounds like some of the patches here depend on others.  If that is
the case, the mutually-dependent parts should be in a single patch, to
facilitate the review.

Stefan, WDYT about these issues?