Bug#35738548: SEGV (Item_ref::real_item() at sql/item.h:5825)

Chaithra Gopalareddy · dahlerlend · commit 649c13266753 · 2023-12-08T08:40:23.000+01:00
found by fuzzing tool
Bug#35779012: SEGV (Item_subselect::print() at
              sql/item_subselect.cc:835) found by fuzzing tool
Bug#35733778: SEGV (Item_subselect::exec() at
              sql/item_subselect.cc:660) found by fuzzing tool
Bug#35738531: Another SEGV (Item_subselect::exec() at
              sql/item_subselect.cc:660) found by fuzzing tool

If an Item_ref object is referenced from multiple
places in a query block, and if the item that it
refers to is also referenced from multiple places,
there is a chance that while removing redundant
expressions, we could end up removing the
referenced item even though it is still being
referred to.
E.g. while resolving ORDER BY clause, if the
expression is found in the select list, expression
used in order by is removed and it starts
using the one in select list. When this happens,
while removing the redundant expressions from the
query block, if the select expression is an
Item_ref object, on the first visit to this
expression, we mark the object as unlinked. On
the second visit, this time because of the
order by, as the object is marked as unlinked,
it exits the traversal without doing anything. However,
when the item it refers to is visited, it does not
know that the item is still being referred to. So
it ends up deleting the referenced item.

Solution is to decrement the ref_count of
an item without initiating the clean up
of the underlying item unless its the last
reference (This necessitated changes to
all implementations of clean_up_after_removal).
Along with this we also remove m_unlinked member
because it's no more needed. If the underlying
item of an Item_ref object is not abandoned, we
decrement the ref count and stop looking further.

Change-Id: I4ef3aaf92a8c0961a541dae09c766929d93bb64e
diff --git a/sql/item.cc b/sql/item.cc
@@ -7562,6 +7562,18 @@ bool Item::cache_const_expr_analyzer(uchar **arg) {
   return false;
 }
 
+bool Item::clean_up_after_removal(uchar *arg) {
+  Cleanup_after_removal_context *const ctx =
+      pointer_cast<Cleanup_after_removal_context *>(arg);
+
+  if (ctx->is_stopped(this)) return false;
+
+  if (reference_count() > 1) {
+    (void)decrement_ref_count();
+  }
+  return false;
+}
+
 bool Item::can_be_substituted_for_gc(bool array) const {
   switch (real_item()->type()) {
     case FUNC_ITEM:
@@ -7977,16 +7989,19 @@ bool Item_ref::clean_up_after_removal(uchar *arg) {
 
   if (ctx->is_stopped(this)) return false;
 
-  // Exit if second visit to this object:
-  if (m_unlinked) return false;
+  // Decrement reference count for referencing object before
+  // referenced object:
+  if (reference_count() > 1) {
+    (void)decrement_ref_count();
+    ctx->stop_at(this);
+    return false;
+  }
+  if (ref_item()->is_abandoned()) return false;
 
   if (ref_item()->decrement_ref_count() > 0) {
     ctx->stop_at(this);
   }
 
-  // Ensure the count is not decremented twice:
-  m_unlinked = true;
-
   return false;
 }
 
diff --git a/sql/item.h b/sql/item.h
@@ -2732,6 +2732,7 @@ class Item : public Parse_tree_node {
     */
     Query_block *const m_root;
 
+    friend class Item;
     friend class Item_sum;
     friend class Item_subselect;
     friend class Item_ref;
@@ -2740,11 +2741,12 @@ class Item : public Parse_tree_node {
      Clean up after removing the item from the item tree.
 
      param arg pointer to a Cleanup_after_removal_context object
+     @todo: If class ORDER is refactored so that all indirect
+     grouping/ordering expressions are represented with Item_ref
+     objects, all implementations of cleanup_after_removal() except
+     the one for Item_ref can be removed.
   */
-  virtual bool clean_up_after_removal(uchar *arg [[maybe_unused]]) {
-    assert(arg != nullptr);
-    return false;
-  }
+  virtual bool clean_up_after_removal(uchar *arg);
 
   /// @see Distinct_check::check_query()
   virtual bool aggregate_check_distinct(uchar *) { return false; }
@@ -3209,6 +3211,9 @@ class Item : public Parse_tree_node {
   */
   bool is_blob_field() const;
 
+  /// @returns number of references to an item.
+  uint reference_count() const { return m_ref_count; }
+
   /// Increment reference count
   void increment_ref_count() {
     assert(!m_abandoned);
@@ -3338,6 +3343,8 @@ class Item : public Parse_tree_node {
   }
   virtual bool strip_db_table_name_processor(uchar *) { return false; }
 
+  bool is_abandoned() const { return m_abandoned; }
+
  private:
   virtual bool subq_opt_away_processor(uchar *) { return false; }
 
@@ -5712,9 +5719,6 @@ class Item_ref : public Item_ident {
   bool pusheddown_depended_from{false};
 
  private:
-  /// True if referenced item has been unlinked, used during item tree removal
-  bool m_unlinked{false};
-
   Field *result_field{nullptr}; /* Save result here */
 
  protected:
diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc
@@ -2683,6 +2683,12 @@ bool Item_subselect::clean_up_after_removal(uchar *arg) {
   // Check whether this item should be removed
   if (ctx->is_stopped(this)) return false;
 
+  if (reference_count() > 1) {
+    (void)decrement_ref_count();
+    ctx->stop_at(this);
+    return false;
+  }
+
   // Remove item on upward traversal, not downward:
   if (marker == MARKER_NONE) {
     marker = MARKER_TRAVERSAL;
diff --git a/sql/item_sum.cc b/sql/item_sum.cc
@@ -524,6 +524,12 @@ bool Item_sum::clean_up_after_removal(uchar *arg) {
 
   if (ctx->is_stopped(this)) return false;
 
+  if (reference_count() > 1) {
+    (void)decrement_ref_count();
+    ctx->stop_at(this);
+    return false;
+  }
+
   // Remove item on upward traversal, not downward:
   if (marker == MARKER_NONE) {
     marker = MARKER_TRAVERSAL;
