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

should not pass recv_writer_thread_handle to CloseHandle()

        Details

          Description

          Calling CloseHandle on an invalid handle. See here:

          UNIV_INTERN os_thread_t recv_writer_thread_handle = 0;

          recv_init_crash_recovery() does:

          recv_writer_thread_handle = os_thread_create(recv_writer_thread, 0, 0);

          recv_recovery_from_checkpoint_finish() does:

          if (recv_writer_thread_handle)

          { CloseHandle(recv_writer_thread_handle); }

          However, os_thread_create() is not returning the HANDLE object from CreateThread, it is returning the lpThreadId!!!

            Gliffy Diagrams

              Attachments

                Activity

                Ascending order - Click to sort in descending order
                Hide
                Permalink
                sbester1 sbester1 added a comment -

                Worst case scenario, crash recovery closes some actual valid file or thread handle and failure would be mysterious!
                Best case, the handle doesn't exist and issue goes unnoticed.

                Show
                sbester1 sbester1 added a comment - Worst case scenario, crash recovery closes some actual valid file or thread handle and failure would be mysterious! Best case, the handle doesn't exist and issue goes unnoticed.
                Hide
                Permalink
                sbester1 sbester1 added a comment -

                Same problem here in fts_parallel_tokenization():
                #ifdef _WIN_
                CloseHandle(psort_info->thread_hdl);
                #endif /*_WIN_ */

                Show
                sbester1 sbester1 added a comment - Same problem here in fts_parallel_tokenization(): #ifdef _ WIN _ CloseHandle(psort_info->thread_hdl); #endif /*_ WIN _ */
                Hide
                Permalink
                sbester1 sbester1 added a comment -

                Offending patch:
                http://bazaar.launchpad.net/~maria-captains/maria/5.5/revision/3449

                Show
                sbester1 sbester1 added a comment - Offending patch: http://bazaar.launchpad.net/~maria-captains/maria/5.5/revision/3449
                Hide
                Permalink
                jplindst Jan Lindström added a comment -

                revno: 4549
                committer: Jan Lindström <jplindst@mariadb.org>
                branch nick: 10.0-innodb
                timestamp: Tue 2015-01-06 16:08:42 +0200
                message:
                MDEV-7403: should not pass recv_writer_thread_handle to CloseHandle()

                Analysis: For some reason actual thread handle is not
                returned on Windows instead lpThreadId was returned and
                thread handle was closed after thread create. Later
                CloseHandle was called for recv_writer_thread_handle
                and psort_info->thread_hdl.

                Fix: Return thread handle from os_thread_create()
                also on Windows and store these thread handles also
                in srv0start.cc so that they can be later closed.

                Show
                jplindst Jan Lindström added a comment - revno: 4549 committer: Jan Lindström <jplindst@mariadb.org> branch nick: 10.0-innodb timestamp: Tue 2015-01-06 16:08:42 +0200 message: MDEV-7403 : should not pass recv_writer_thread_handle to CloseHandle() Analysis: For some reason actual thread handle is not returned on Windows instead lpThreadId was returned and thread handle was closed after thread create. Later CloseHandle was called for recv_writer_thread_handle and psort_info->thread_hdl. Fix: Return thread handle from os_thread_create() also on Windows and store these thread handles also in srv0start.cc so that they can be later closed.
                Hide
                Permalink
                elenst Elena Stepanova added a comment -

                The fix was pushed into 10.0, should the bug now be closed?
                On the other hand, the fix came without any regression tests, is it possible at all to get the changed code tested by MTR? (gcov complained about it, see http://buildbot.askmonty.org/buildbot/builders/kvm-dgcov-jaunty-i386/builds/4542).

                Show
                elenst Elena Stepanova added a comment - The fix was pushed into 10.0, should the bug now be closed? On the other hand, the fix came without any regression tests, is it possible at all to get the changed code tested by MTR? (gcov complained about it, see http://buildbot.askmonty.org/buildbot/builders/kvm-dgcov-jaunty-i386/builds/4542 ).
                Hide
                Permalink
                jplindst Jan Lindström added a comment -

                Actual fix is tested on Windows only, and last build did not test the code at all. Above complain is because by default there is no several purge threads.

                Show
                jplindst Jan Lindström added a comment - Actual fix is tested on Windows only, and last build did not test the code at all. Above complain is because by default there is no several purge threads.

                  People

                  • Assignee:
                    jplindst Jan Lindström
                    Reporter:
                    sbester1 sbester1
                  • Votes:
                    0 Vote for this issue
                    Watchers:
                    4 Start watching this issue

                    Dates

                    • Created:
                      Updated:
                      Resolved: