diagnostics: fix crash in urlifier with -Wfatal-errors [PR119366]

diagnostic_context's dtor assumed that it owned the m_urlifier pointer
and would delete it.

As of r15-5988-g5a022062d22e0b this isn't always the case -
auto_urlify_attributes is used in various places in the C/C++ frontends
and in the middle-end to temporarily override the urlifier with an
on-stack instance, which would lead to delete-of-on-stack-buffer crashes
with -Wfatal-errors as the global_dc was cleaned up.

Fix by explicitly tracking the stack of urlifiers within
diagnostic_context, tracking for each node whether the pointer is
owned or borrowed.

gcc/ChangeLog:
	PR c/119366
	* diagnostic-format-sarif.cc (test_message_with_embedded_link):
	Convert diagnostic_context from one urlifier to a stack of
	urlifiers, where each node in the stack tracks whether the
	urlifier is owned or borrowed.
	* diagnostic.cc (diagnostic_context::initialize): Likewise.
	(diagnostic_context::finish): Likewise.
	(diagnostic_context::set_urlifier): Delete.
	(diagnostic_context::push_owned_urlifier): New.
	(diagnostic_context::push_borrowed_urlifier): New.
	(diagnostic_context::pop_urlifier): New.
	(diagnostic_context::get_urlifier): Reimplement in terms of stack.
	(diagnostic_context::override_urlifier): Delete.
	* diagnostic.h (diagnostic_context::set_urlifier): Delete decl.
	(diagnostic_context::override_urlifier): Delete decl.
	(diagnostic_context::push_owned_urlifier): New decl.
	(diagnostic_context::push_borrowed_urlifier): New decl.
	(diagnostic_context::pop_urlifier): New decl.
	(diagnostic_context::get_urlifier): Make return value const; hide
	implementation.
	(diagnostic_context::m_urlifier): Replace with...
	(diagnostic_context::urlifier_stack_node): ... this and...
	(diagnostic_context::m_urlifier_stack): ...this.
	* gcc-urlifier.cc
	(auto_override_urlifier::auto_override_urlifier): Reimplement.
	(auto_override_urlifier::~auto_override_urlifier): Reimplement.
	* gcc-urlifier.h (class auto_override_urlifier): Reimplement.
	(auto_urlify_attributes::auto_urlify_attributes): Update for
	pass-by-reference.
	* gcc.cc (driver::global_initializations): Update for
	reimplementation of urlifiers in terms of a stack.
	* toplev.cc (general_init): Likewise.

gcc/testsuite/ChangeLog:
	PR c/119366
	* gcc.dg/Wfatal-bad-attr-pr119366.c: New test.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>

diff --git a/gcc/diagnostic-format-sarif.cc b/gcc/diagnostic-format-sarif.cc
index 554992bddba1c7670bf4db49966e492315a2aa12..8dbc91ee8f32b3f708ba976ad6b411689e457723 100644 (file)
--- a/gcc/diagnostic-format-sarif.cc
+++ b/gcc/diagnostic-format-sarif.cc
@@ -4365,7 +4365,7 @@ test_message_with_embedded_link (enum sarif_version version)
     };
 
     test_sarif_diagnostic_context dc ("test.c", version);
-    dc.set_urlifier (::make_unique<test_urlifier> ());
+    dc.push_owned_urlifier (::make_unique<test_urlifier> ());
     rich_location richloc (line_table, UNKNOWN_LOCATION);
     dc.report (DK_ERROR, richloc, nullptr, 0,
               "foo %<-foption%> %<unrecognized%> bar");

diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
index c2f6714c24aa6fddcb1c87159648442fa45418dd..82d7f946818b2982cf9c11d4a3e6920034290676 100644 (file)
--- a/gcc/diagnostic.cc
+++ b/gcc/diagnostic.cc
@@ -254,7 +254,7 @@ diagnostic_context::initialize (int n_opts)
   m_text_callbacks.m_start_span = default_diagnostic_start_span_fn;
   m_text_callbacks.m_end_diagnostic = default_diagnostic_text_finalizer;
   m_option_mgr = nullptr;
-  m_urlifier = nullptr;
+  m_urlifier_stack = new auto_vec<urlifier_stack_node> ();
   m_last_location = UNKNOWN_LOCATION;
   m_client_aux_data = nullptr;
   m_lock = 0;
@@ -424,8 +424,13 @@ diagnostic_context::finish ()
   delete m_option_mgr;
   m_option_mgr = nullptr;
 
-  delete m_urlifier;
-  m_urlifier = nullptr;
+  if (m_urlifier_stack)
+    {
+      while (!m_urlifier_stack->is_empty ())
+       pop_urlifier ();
+      delete m_urlifier_stack;
+      m_urlifier_stack = nullptr;
+    }
 
   freeargv (m_original_argv);
   m_original_argv = nullptr;
@@ -549,13 +554,43 @@ set_option_manager (std::unique_ptr<diagnostic_option_manager> mgr,
 }
 
 void
-diagnostic_context::set_urlifier (std::unique_ptr<urlifier> urlifier)
+diagnostic_context::push_owned_urlifier (std::unique_ptr<urlifier> ptr)
 {
-  delete m_urlifier;
-  /* Ideally the field would be a std::unique_ptr here.  */
-  m_urlifier = urlifier.release ();
+  gcc_assert (m_urlifier_stack);
+  const urlifier_stack_node node = { ptr.release (), true };
+  m_urlifier_stack->safe_push (node);
 }
 
+void
+diagnostic_context::push_borrowed_urlifier (const urlifier &loan)
+{
+  gcc_assert (m_urlifier_stack);
+  const urlifier_stack_node node = { const_cast <urlifier *> (&loan), false };
+  m_urlifier_stack->safe_push (node);
+}
+
+void
+diagnostic_context::pop_urlifier ()
+{
+  gcc_assert (m_urlifier_stack);
+  gcc_assert (m_urlifier_stack->length () > 0);
+
+  const urlifier_stack_node node = m_urlifier_stack->pop ();
+  if (node.m_owned)
+    delete node.m_urlifier;
+}
+
+const urlifier *
+diagnostic_context::get_urlifier () const
+{
+  if (!m_urlifier_stack)
+    return nullptr;
+  if (m_urlifier_stack->is_empty ())
+    return nullptr;
+  return m_urlifier_stack->last ().m_urlifier;
+}
+
+
 /* Set PP as the reference printer for this context.
    Refresh all output sinks.  */
 
@@ -605,14 +640,6 @@ diagnostic_context::set_prefixing_rule (diagnostic_prefixing_rule_t rule)
       pp_prefixing_rule (sink->get_printer ()) = rule;
 }
 
-/* Set the urlifier without deleting the existing one.  */
-
-void
-diagnostic_context::override_urlifier (urlifier *urlifier)
-{
-  m_urlifier = urlifier;
-}
-
 void
 diagnostic_context::create_edit_context ()
 {

diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 202760b2f85d2d2a04aba134fa0150dc0b8129c4..62bffd2c6851a3173a4f1ef2da6f918abd07de74 100644 (file)
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -609,8 +609,11 @@ public:
   void set_output_format (std::unique_ptr<diagnostic_output_format> output_format);
   void set_text_art_charset (enum diagnostic_text_art_charset charset);
   void set_client_data_hooks (std::unique_ptr<diagnostic_client_data_hooks> hooks);
-  void set_urlifier (std::unique_ptr<urlifier>);
-  void override_urlifier (urlifier *);
+
+  void push_owned_urlifier (std::unique_ptr<urlifier>);
+  void push_borrowed_urlifier (const urlifier &);
+  void pop_urlifier ();
+
   void create_edit_context ();
   void set_warning_as_error_requested (bool val)
   {
@@ -667,7 +670,8 @@ public:
   {
     return m_client_data_hooks;
   }
-  urlifier *get_urlifier () const { return m_urlifier; }
+
+  const urlifier *get_urlifier () const;
 
   text_art::theme *get_diagram_theme () const { return m_diagrams.m_theme; }
 
@@ -888,11 +892,16 @@ private:
   diagnostic_option_manager *m_option_mgr;
   unsigned m_lang_mask;
 
-  /* An optional hook for adding URLs to quoted text strings in
+  /* A stack of optional hooks for adding URLs to quoted text strings in
      diagnostics.  Only used for the main diagnostic message.
-     Owned by the context; this would be a std::unique_ptr if
-     diagnostic_context had a proper ctor.  */
-  urlifier *m_urlifier;
+     Typically a single one owner by the context, but can be temporarily
+     overridden by a borrowed urlifier (e.g. on-stack).  */
+  struct urlifier_stack_node
+  {
+    urlifier *m_urlifier;
+    bool m_owned;
+  };
+  auto_vec<urlifier_stack_node> *m_urlifier_stack;
 
 public:
   /* Auxiliary data for client.  */

diff --git a/gcc/gcc-urlifier.cc b/gcc/gcc-urlifier.cc
index 49611b71d9b9a74f03c6ccaaba7d60ceac00f7e2..a4094582f416769ee17dd0a0bac1e99267f24ccb 100644 (file)
--- a/gcc/gcc-urlifier.cc
+++ b/gcc/gcc-urlifier.cc
@@ -220,15 +220,14 @@ make_gcc_urlifier (unsigned int lang_mask)
 
 /* class auto_override_urlifier.  */
 
-auto_override_urlifier::auto_override_urlifier (urlifier *new_urlifier)
-: m_old_urlifier (global_dc->get_urlifier ())
+auto_override_urlifier::auto_override_urlifier (const urlifier &new_urlifier)
 {
-  global_dc->override_urlifier (new_urlifier);
+  global_dc->push_borrowed_urlifier (new_urlifier);
 }
 
 auto_override_urlifier::~auto_override_urlifier ()
 {
-  global_dc->override_urlifier (m_old_urlifier);
+  global_dc->pop_urlifier ();
 }
 
 #if CHECKING_P

diff --git a/gcc/gcc-urlifier.h b/gcc/gcc-urlifier.h
index 5ffbbf7a4cebae7ce885c0e8f08115808cd67fc7..eefed49032825183eb33cc59ab2862ebaed87399 100644 (file)
--- a/gcc/gcc-urlifier.h
+++ b/gcc/gcc-urlifier.h
@@ -33,11 +33,8 @@ extern char *make_doc_url (const char *doc_url_suffix);
 class auto_override_urlifier
 {
 public:
-  auto_override_urlifier (urlifier *new_urlifier);
+  auto_override_urlifier (const urlifier &new_urlifier);
   ~auto_override_urlifier ();
-
-protected:
-  urlifier * const m_old_urlifier;
 };
 
 /* Subclass of urlifier that attempts to add URLs to quoted strings
@@ -71,7 +68,7 @@ class auto_urlify_attributes
 {
 public:
   auto_urlify_attributes ()
-  : m_override (&m_urlifier)
+  : m_override (m_urlifier)
   {
   }
 

diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 04b3736a5da17dc3c743ea2ed8a924c83ce5c54a..c7b2aa6df166e57c958c0ff7f14a6f1d953d0bac 100644 (file)
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -8356,7 +8356,7 @@ driver::global_initializations ()
   diagnostic_initialize (global_dc, 0);
   diagnostic_color_init (global_dc);
   diagnostic_urls_init (global_dc);
-  global_dc->set_urlifier (make_gcc_urlifier (0));
+  global_dc->push_owned_urlifier (make_gcc_urlifier (0));
 
 #ifdef GCC_DRIVER_HOST_INITIALIZATION
   /* Perform host dependent initialization when needed.  */

diff --git a/gcc/testsuite/gcc.dg/Wfatal-bad-attr-pr119366.c b/gcc/testsuite/gcc.dg/Wfatal-bad-attr-pr119366.c
new file mode 100644 (file)
index 0000000..2ca4eed
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wfatal-bad-attr-pr119366.c
@@ -0,0 +1,8 @@
+/* Verify that we don't crash if we bail out with a fatal error
+   while an on-stack attribute_urlifier is active (PR c/119366).  */
+/* { dg-options "-Wfatal-errors -Werror=attributes" } */
+void foo() __attribute__((this_does_not_exist)); /* { dg-error "'this_does_not_exist' attribute directive ignored" } */
+
+/* { dg-message "some warnings being treated as errors" "treated as errors" {target "*-*-*"} 0 } */
+/* { dg-message "terminated due to -Wfatal-errors" "terminated" { target *-*-* } 0 } */
+

diff --git a/gcc/toplev.cc b/gcc/toplev.cc
index c5d46ec98015e907295378abc27723b53f5222ae..6d8b8852fb84cc50d5c81080881c526edf2eea44 100644 (file)
--- a/gcc/toplev.cc
+++ b/gcc/toplev.cc
@@ -1099,7 +1099,7 @@ general_init (const char *argv0, bool init_signals, unique_argv original_argv)
                                                        lang_mask,
                                                        &global_options),
      lang_mask);
-  global_dc->set_urlifier (make_gcc_urlifier (lang_mask));
+  global_dc->push_owned_urlifier (make_gcc_urlifier (lang_mask));
 
   if (init_signals)
     {