maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vicentiu!

On Jun 20, Vicentiu wrote:
> revision-id: 9c253aeafdde8331b8d6f7ed11f1209ab4ce1d5b (mariadb-10.0.25-14-g9c253ae)
> parent(s): 9301d5148d68ea7ae89918b87175c374028bfc5d
> author: Vicențiu Ciorbaru
> committer: Vicențiu Ciorbaru
> timestamp: 2016-06-20 23:43:01 +0300
> message:
> 
> MDEV-8636: REVOKE ALL PRIVILEGES, GRANT OPTION FROM CURRENT_ROLE breaks replication

it's 8638, not 8636

> Fix the replication failure caused by incorect initialization of
> THD::invoker_host && THD::invoker_user.
> 
> Breakdown of the failure is this:
> Query_log_event::host and Query_log_event::user can have their
> LEX_STRING's set to length 0, but the actual str member points to
> garbage. Code afterwards copies Query_log_event::host and user to
> THD::invoker_host and THD::invoker_user.
 
> Calling code for these members expects both members to be initialized.
> Eg. the str member be a NULL terminated string and length have
> appropriate size.
> 
> diff --git a/sql/log_event.cc b/sql/log_event.cc
> index 93704e7..4facd4f 100644
> --- a/sql/log_event.cc
> +++ b/sql/log_event.cc
> @@ -3631,10 +3631,8 @@ Query_log_event::Query_log_event(const char* buf, uint event_len,
>    if (time_zone_len)
>      copy_str_and_move(&time_zone_str, &start, time_zone_len);
>  
> -  if (user.length > 0)
> -    copy_str_and_move((const char **)&(user.str), &start, user.length);
> -  if (host.length > 0)
> -    copy_str_and_move((const char **)&(host.str), &start, host.length);
> +  copy_str_and_move((const char **)&(user.str), &start, user.length);
> +  copy_str_and_move((const char **)&(host.str), &start, host.length);

I think the bug is that user.str or host.str may point to some garbage.
I'd suggest the following fix instead:

--- a/sql/log_event.cc
+++ b/sql/log_event.cc
@@ -3548,17 +3548,12 @@ Query_log_event::Query_log_event(const char* buf, uint e
       break;
     case Q_INVOKER:
     {
-      CHECK_SPACE(pos, end, 1);
-      user.length= *pos++;
-      CHECK_SPACE(pos, end, user.length);
-      user.str= (char *)pos;
-      pos+= user.length;
-
-      CHECK_SPACE(pos, end, 1);
-      host.length= *pos++;
-      CHECK_SPACE(pos, end, host.length);
-      host.str= (char *)pos;
-      pos+= host.length;
+      if (get_str_len_and_pointer(&pos, &user.str, &user.length, end) ||
+          get_str_len_and_pointer(&pos, &host.str. &host.length, end))
+      {
+        query= 0;
+        DBUG_VOID_RETURN;
+      }
       break;
     }
     case Q_HRNOW:

Ok ot push if you agree (and it works :)

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx