maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

On Aug 14, Alexey Botchkov wrote:
> revision-id: e2a82094332 (mariadb-10.4.7-123-ge2a82094332)
> parent(s): 691654721b3
> author: Alexey Botchkov <holyfoot@xxxxxxxxxxx>
> committer: Alexey Botchkov <holyfoot@xxxxxxxxxxx>
> timestamp: 2019-08-07 11:49:54 +0400
> message:
> 
> MENT-253 Server Audit v2 does not work with PS protocol: server crashes in filter_query_type.
> 
> Set the thd->query_string where possible.
> server_audit2 should not crash when gets empty event->query.
> 
> ---
>  plugin/server_audit2/server_audit.c | 15 ++++++++++++---
>  sql/sql_prepare.cc                  | 22 ++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 3 deletions(-)

Test case? With normal and error code paths?

> diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> index 74723d5bd91..d2cb4e9b372 100644
> --- a/sql/sql_prepare.cc
> +++ b/sql/sql_prepare.cc
> @@ -2600,6 +2600,14 @@ void mysqld_stmt_prepare(THD *thd, const char *packet, uint packet_length)
>  
>    if (stmt->prepare(packet, packet_length))
>    {
> +    /*
> +       Set the thd->query_string with the current query so the
> +       audit plugin gets the meaningful notification.

say that it's an error case. Like

          Prepare failed.
          Set the thd->query_string with the current query so the
          audit plugin gets the meaningful notification when it gets
          the error notification.

> +    */
> +    if (alloc_query(thd, stmt->query_string.str(), stmt->query_string.length()))
> +    {
> +      thd->set_query(0, 0);
> +    }
>      /* Statement map deletes statement on erase */
>      thd->stmt_map.erase(stmt);
>      thd->clear_last_stmt();
> @@ -3184,6 +3192,13 @@ static void mysql_stmt_execute_common(THD *thd,
>      char llbuf[22];
>      my_error(ER_UNKNOWN_STMT_HANDLER, MYF(0), static_cast<int>(sizeof(llbuf)),
>               llstr(stmt_id, llbuf), "mysqld_stmt_execute");
> +    /*
> +      Set thd->query_string with the stmt_id so the
> +      audit plugin gets the meaningful notification.

Same here.

> +    */
> +    if (alloc_query(thd, llbuf, strlen(llbuf)))
> +      thd->set_query(0, 0);
> +
>      DBUG_VOID_RETURN;
>    }
>    stmt->read_types= read_types;
> @@ -3939,6 +3954,13 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len)
>      DBUG_RETURN(TRUE);
>    }
>  
> +  /*
> +    We'd like to have thd->query to be set to the actual query
> +    after the function ends.
> +    This value will be sent to audit pulgins later.

1. typo: "pulgins"

2. Mention explicitly that "query string is allocated in the stmt arena,
   not in the thd arena. But it's safe here, because stmt always has a longer
   lifetime than thd arena." - or something along these lines.

> +  */
> +  stmt_backup.query_string= thd->query_string;
> +
>    old_stmt_arena= thd->stmt_arena;
>    thd->stmt_arena= this;
>  
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx