Skip to content

MDEV-28509: Dereferenced null pointer of type 'struct JOIN_TAB' in ad…#5346

Open
DaveGosselin-MariaDB wants to merge 1 commit into
10.11from
10.11-mdev-28509-addkeyfield-crash
Open

MDEV-28509: Dereferenced null pointer of type 'struct JOIN_TAB' in ad…#5346
DaveGosselin-MariaDB wants to merge 1 commit into
10.11from
10.11-mdev-28509-addkeyfield-crash

Conversation

@DaveGosselin-MariaDB

Copy link
Copy Markdown
Member

…d_key_field

This patch fixes a crash when calculating join statistics during query optimization for queries with an unused WINDOW definition. Put another way, the system may crash when a query defines a WINDOW but doesn't then refer to it.

Item::marker is overloaded for different uses, many of which treat it as a bit field. However, the setup_group function uses it to mark that a field was found when traversing a GROUP BY. Originally, this marking set the Item::marker field to 1 to indicate that it was found. Later on in setup_group (and only when SQL mode ONLY_FULL_GROUP_BY is enabled), we would skip any such marked fields when checking that fields only referenced those found in the GROUP BY; otherwise, it would be silly to find fields of the GROUP BY within the GROUP BY field itself. Setting Item::marker to 1 seemed mostly harmless at that point in time. But later, in git sha 4d143a6, we introduced several changes: (1) the value of marker in setup_group was changed from 1 to UNDEF_POS, (2) Item::marker was changed from uint8 to int8 (which has since been changed to an int), and (3) UNDEF_POS which is defined to be -1 was also added.

Queries that define WINDOWs internally will setup groups and orders as part of query processing via the setup_group function. Consequently because of the behavior described earlier above, such queries may have items with markers as UNDEF_POS (-1, or 0xffffffff) which is the same as marking all of the flag bits as set. This is disastrous for those users of Item::marker which refer to it as a bit field, because every flag bit appears set at once, including bits that are mutually exclusive in meaning. Even a masked test such as marker & SUBSTITUTION_FL returns true when marker is -1. In particular, the method Item_direct_view_ref::grouping_field_transformer_for_where then treats the ref as flagged for substitution and takes the wrong execution path, leading to the crash.

Upon return from the setup_group function, we set the value of the marker flag to zero if we set it to -1. This preserves the marker behavior for 'full group by' (if configured) while not otherwise allowing the marker flag state to leak outside of this function.

…d_key_field

This patch fixes a crash when calculating join statistics during query
optimization for queries with an unused WINDOW definition.  Put another
way, the system may crash when a query defines a WINDOW but doesn't then
refer to it.

Item::marker is overloaded for different uses, many of which treat it as
a bit field.  However, the setup_group function uses it to mark that a
field was found when traversing a GROUP BY.  Originally, this marking
set the Item::marker field to 1 to indicate that it was found.  Later
on in setup_group (and only when SQL mode ONLY_FULL_GROUP_BY is
enabled), we would skip any such marked fields when checking that
fields only referenced those found in the GROUP BY; otherwise, it would
be silly to find fields of the GROUP BY within the GROUP BY field
itself.  Setting Item::marker to 1 seemed mostly harmless at that point
in time.  But later, in git sha 4d143a6, we introduced several
changes: (1) the value of marker in setup_group was changed from 1 to
UNDEF_POS, (2) Item::marker was changed from uint8 to int8 (which has
since been changed to an int), and (3) UNDEF_POS which is defined to be
-1 was also added.

Queries that define WINDOWs internally will setup groups and orders as
part of query processing via the setup_group function.  Consequently
because of the behavior described earlier above, such queries may have
items with markers as UNDEF_POS (-1, or 0xffffffff) which is the same
as marking all of the flag bits as set.  This is disastrous for those
users of Item::marker which refer to it as a bit field, because every
flag bit appears set at once, including bits that are mutually
exclusive in meaning.  Even a masked test such as marker &
SUBSTITUTION_FL returns true when marker is -1.  In particular, the
method Item_direct_view_ref::grouping_field_transformer_for_where then
treats the ref as flagged for substitution and takes the wrong
execution path, leading to the crash.

Upon return from the setup_group function, we set the value of the
marker flag to zero if we set it to -1.  This preserves the marker
behavior for 'full group by' (if configured) while not otherwise
allowing the marker flag state to leak outside of this function.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds test cases to verify the fix for MDEV-28509, which addresses a null pointer access issue during the add_key_field call. The tests cover scenarios involving Common Table Expressions (CTEs) with window functions, subqueries with GROUP BY, and various WHERE clause conditions under different SQL modes. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

1 participant