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?