Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-35646

pseudo_thread_id Can Overflow on Binlog Write

Details

    Description

      Though the thread_id type is 8 bytes long, it can't actually have a value larger than 4 bytes (MDEV-15089). However, the global variable pseudo_thread_id is 8 bytes long, can be set dynamically, and when so, only the first 4 bytes are written to the thread_id field of a binary log's Query_log_event and Gtid_log_event (Gtid_log_event debuting in 11.5 with MDEV-7850). So if a user sets the pseudo_thread_id to be larger than 4 bytes, it is truncated. The value should be restricted to be within the 4-byte range.

      Attachments

        Issue Links

          Activity

            ParadoxV5 Jimmy Hú added a comment - - edited

            In mysqld.cc, static recalculate_thread_id_range() places a grand limit of (0, UINT_MAX32) on my_thread_id next_thread_id(void) and aborts the program when there’re “2^32 connections”.
            Then why is my_thread_id a typedef of uint64, pseudo- or not?

            ParadoxV5 Jimmy Hú added a comment - - edited In mysqld.cc , static recalculate_thread_id_range() places a grand limit of (0, UINT_MAX32) on my_thread_id next_thread_id(void) and aborts the program when there’re “2^32 connections”. Then why is my_thread_id a typedef of uint64 , pseudo - or not?
            ParadoxV5 Jimmy Hú added a comment - https://github.com/MariaDB/server/pull/3721
            wlad Vladislav Vaintroub added a comment - - edited

            my_thread_id is de-facto UINT32, because in the client-server protocol there are only 32bit reserved for connection id, and if we change this 32bit to 64bit, everything will break (ok, not everything, but at the very least the ability to KILL current connection, because 32bit is engrained into every possible connector). There were some ideas around 64bits , once people realized overflow is possible, but luckily those ideas were not implemented, instead an efficient backward-compatible next_thread_id() was implemented.

            The last change for the typedef was from ulong (sigh) to uint64 in b3093073b1ec076ad7ef5ef8b8fca9022864a890 to fix further unspecified "compilation problem with my_atomic_add32_explicit on windows".

            There is no reason why my_thread_id remains uint64, and uint32 is definitely more logical, but changing that type internally would certainly would extend MDEV-15089 scope from small to huge patch. Correcting typedef was left "for the future", since it did not bother anyone. Now , since it bothers someone, this "future" has come

            wlad Vladislav Vaintroub added a comment - - edited my_thread_id is de-facto UINT32, because in the client-server protocol there are only 32bit reserved for connection id, and if we change this 32bit to 64bit, everything will break (ok, not everything, but at the very least the ability to KILL current connection, because 32bit is engrained into every possible connector). There were some ideas around 64bits , once people realized overflow is possible, but luckily those ideas were not implemented, instead an efficient backward-compatible next_thread_id() was implemented. The last change for the typedef was from ulong (sigh) to uint64 in b3093073b1ec076ad7ef5ef8b8fca9022864a890 to fix further unspecified "compilation problem with my_atomic_add32_explicit on windows". There is no reason why my_thread_id remains uint64, and uint32 is definitely more logical, but changing that type internally would certainly would extend MDEV-15089 scope from small to huge patch. Correcting typedef was left "for the future", since it did not bother anyone. Now , since it bothers someone, this "future" has come
            ParadoxV5 Jimmy Hú added a comment -

            This future is overdue. #tech_debt

            ParadoxV5 Jimmy Hú added a comment - This future is overdue. #tech_debt

            Thank you for the patch, ParadoxV5! I left a note on the PR.

            bnestere Brandon Nesterenko added a comment - Thank you for the patch, ParadoxV5 ! I left a note on the PR.

            Patch is approved (with a small note about a comment)

            bnestere Brandon Nesterenko added a comment - Patch is approved (with a small note about a comment)
            ParadoxV5 Jimmy Hú added a comment - bnestere Are we good to go? ( How does the merging process go anyway, just hit the “Enable auto-merge (rebase)” button? )

            ParadoxV5 yep, you are all good to push!

            bnestere Brandon Nesterenko added a comment - ParadoxV5 yep, you are all good to push!

            People

              ParadoxV5 Jimmy Hú
              bnestere Brandon Nesterenko
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.