[PATCHv2 2/2] gdb: add debug_file_directory changed observer

[Andrew Burgess <aburgess@redhat.com>]

Add a new observer gdb::observers::gdb_debugdir_changed; this is
triggered whenever the debug_file_directory is updated.

This new observer is analogous to the existing gdb_datadir_changed
observer.

I was motivated to add this when trying to understand some code in
auto-load.c.  In file_is_auto_load_safe we call
filename_is_in_auto_load_safe_path_vec, and if this fails we call
auto_load_safe_path_vec_update, then
filename_is_in_auto_load_safe_path_vec again.  This seemed weird to
me, why would we need to update the safe path vector again at this
point?

I think there are two reasons why this might be needed (before this
patch):

 1. If the definition of $debugdir changes then the vector needs to be
    rebuilt based on the new value, and

 2. If any symlinks on the filesystem change the gdb_realpath might
    return new directory or filenames, which would then cause
    different things to match.

The first case here is similar to $datadir, but for $datadir we have
an observer which triggers an update of the safe path vector as soon
as the variable changes.  I figure that it makes sense to add an
observer for the $debugdir case too.

This leaves just case (2) as being the motivation for rebuilding the
vector in the auto-load code.  I've also added a comment about this
to the code.

I extended the existing auto-load-after-debug-dir-change.exp test to
cover this case.

The setup for the new test is that we set the auto-load safe path to
'$debugdir', then set $debugdir to a directory D1 and cause GDB to
compute the auto-load safe path vector.  We can then set the auto load
script path to the same D1 directory.

At this point GDB will look for auto load script in D1, find that
these match the auto-load safe path, and load them.

Now if we change debug file directory (aka $debugdir) to a different
directory D2 and try loading an executable, GDB will use the cached
auto-load safe path which still contains D1, GDB will incorrectly load
the script when the script should be rejected.

With this patch the script will be rejected as expected.
---
 gdb/auto-load.c                               | 18 +++++++--
 gdb/observable.c                              |  1 +
 gdb/observable.h                              |  4 ++
 gdb/symfile.c                                 | 12 +++++-
 .../auto-load-after-debug-dir-change.exp      | 39 +++++++++++++++----
 5 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index 371d24c2b37..8b597c8070b 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -298,10 +298,11 @@ auto_load_safe_path_vec_update (void)
     }
 }
 
-/* Variable gdb_datadir has been set.  Update content depending on $datadir.  */
+/* One of the variables gdb_datadir or debug_file_directory might have
+   changed.  Update content depending on $datadir and $debugdir.  */
 
 static void
-auto_load_gdb_datadir_changed (void)
+auto_load_gdb_vars_changed ()
 {
   auto_load_safe_path_vec_update ();
 }
@@ -503,6 +504,15 @@ file_is_auto_load_safe (const char *filename)
   if (filename_is_in_auto_load_safe_path_vec (filename, &filename_real))
     return true;
 
+  /* The reason for rebuilding the vector, and retrying the match at
+     this point is in case the user has updated some symlinks on their
+     file system.  Rebuilding the safe path vector calls gdb_realpath
+     on each entry and adds the result to the vector.  As a result, if
+     any symlinks change then this update will change the set of paths
+     we consider safe.
+
+     This does mean that a stale entry might trigger a match in the
+     above check, for now though we just accept this risk.  */
   auto_load_safe_path_vec_update ();
   if (filename_is_in_auto_load_safe_path_vec (filename, &filename_real))
     return true;
@@ -1677,7 +1687,9 @@ This option has security implications for untrusted inferiors."),
 				     show_auto_load_safe_path,
 				     auto_load_set_cmdlist_get (),
 				     auto_load_show_cmdlist_get ());
-  gdb::observers::gdb_datadir_changed.attach (auto_load_gdb_datadir_changed,
+  gdb::observers::gdb_datadir_changed.attach (auto_load_gdb_vars_changed,
+					      "auto-load");
+  gdb::observers::gdb_debugdir_changed.attach (auto_load_gdb_vars_changed,
 					      "auto-load");
 
   cmd = add_cmd ("add-auto-load-safe-path", class_support,
diff --git a/gdb/observable.c b/gdb/observable.c
index 151014590a8..6a658303ee9 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -63,6 +63,7 @@ DEFINE_OBSERVABLE (inferior_cloned);
 DEFINE_OBSERVABLE (memory_changed);
 DEFINE_OBSERVABLE (before_prompt);
 DEFINE_OBSERVABLE (gdb_datadir_changed);
+DEFINE_OBSERVABLE (gdb_debugdir_changed);
 DEFINE_OBSERVABLE (inferior_call_pre);
 DEFINE_OBSERVABLE (inferior_call_post);
 DEFINE_OBSERVABLE (register_changed);
diff --git a/gdb/observable.h b/gdb/observable.h
index 979e9d1c3ac..0581b48b711 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -213,6 +213,10 @@ extern observable<const char */* current_prompt */> before_prompt;
    change.  */
 extern observable<> gdb_datadir_changed;
 
+/* Variable debug_file_directory has been set.  The value may not
+   necessarily change.  */
+extern observable<> gdb_debugdir_changed;
+
 /* An inferior function at ADDRESS is about to be called in thread
    THREAD.  */
 extern observable<ptid_t /* thread */, CORE_ADDR /* address */>
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 017f7a49d8d..f8a7378120f 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1340,6 +1340,16 @@ show_debug_file_directory (struct ui_file *file, int from_tty,
 	      value);
 }
 
+/* Implement 'set debug-file-directory' command.  */
+
+static void
+set_debug_file_directory (const char *args, int from_tty,
+			  struct cmd_list_element *e)
+{
+  /* The new value has already been installed by this point.  */
+  gdb::observers::gdb_debugdir_changed.notify ();
+}
+
 #if ! defined (DEBUG_SUBDIRECTORY)
 #define DEBUG_SUBDIRECTORY ".debug"
 #endif
@@ -3885,7 +3895,7 @@ Separate debug symbols are first searched for in the same\n\
 directory as the binary, then in the `" DEBUG_SUBDIRECTORY "' subdirectory,\n\
 and lastly at the path of the directory of the binary with\n\
 each global debug-file-directory component prepended."),
-				     NULL,
+				     set_debug_file_directory,
 				     show_debug_file_directory,
 				     &setlist, &showlist);
 
diff --git a/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change.exp b/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change.exp
index 6cb7a0a8432..4bcdb5670a2 100644
--- a/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change.exp
+++ b/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change.exp
@@ -76,16 +76,37 @@ proc run_test_1 { search_num debug_num } {
 
     gdb_load ${::binfile}
 
-    gdb_test {print $script_loaded} " = ${search_num}00"
+    if { $search_num == $debug_num } {
+	set expected_num "${search_num}00"
+    } else {
+	set expected_num "0"
+    }
 
-    # Check that the expected script shows as auto-loaded in GDB's output.
-    gdb_assert {[regexp "script_${search_num}_loaded" $::gdb_file_cmd_msg]} \
-	"expected script was loaded"
+    gdb_test {print $script_loaded} " = ${expected_num}"
 
-    # The '3 - SEARCH_NUM' converts: 2 => 1 and 1 => 2.  This checks
-    # that the other script was not auto-loaded.
-    gdb_assert {![regexp "script_[expr {3 - $search_num}]_loaded" $::gdb_file_cmd_msg]} \
-	"other script was not loaded"
+    if { $search_num == $debug_num } {
+	# Check that the expected script shows as auto-loaded in GDB's output.
+	gdb_assert {[regexp "script_${search_num}_loaded" \
+			 $::gdb_file_cmd_msg]} \
+	    "expected script was loaded"
+
+	# The '3 - SEARCH_NUM' converts: 2 => 1 and 1 => 2.  This
+	# checks that the other script was not auto-loaded.
+	gdb_assert {![regexp "script_[expr {3 - $search_num}]_loaded" \
+			  $::gdb_file_cmd_msg]} \
+	    "other script was not loaded"
+    } else {
+	set re "warning: File \[^\r\n\]+/debug_dir_${search_num}/\[^\r\n\]+ auto-loading has been declined"
+	gdb_assert {[regexp $re $::gdb_file_cmd_msg]} \
+	    "saw script rejected message"
+
+	gdb_assert {![regexp "script_1_loaded" \
+			  $::gdb_file_cmd_msg]} \
+	    "script 1 was not loaded"
+	gdb_assert {![regexp "script_2_loaded" \
+			  $::gdb_file_cmd_msg]} \
+	    "script 2 was not loaded"
+    }
 }
 
 # Wrapper around run_test_1 which adds a test prefix.  See run_test_1
@@ -99,3 +120,5 @@ proc run_test { search_num debug_num } {
 # Run the tests.
 run_test 1 1
 run_test 2 2
+run_test 2 1
+run_test 1 2
-- 
2.25.4