Details

    Description

      Instrument MEM_ROOT and mem_heap_t.

      Safe malloc disabled for WITH_ASAN=ON.

      With this patch ASAN is able to catch eg this bug

      Also remove unneeded header inclusions.

      Attachments

        Issue Links

          Activity

            svoj Sergey Vojtovich created issue -
            serg Sergei Golubchik added a comment - - edited

            Mostly already implemented with these commits:

            c3e25703c75 improve ASAN instrumentation: clang
            03eb15933da improve ASAN instrumentation: InnoDB/XtraDB
            f2408e7e6a3 Free memory in unit tests. Makes ASAN happier.
            36eb0b7a558 improve ASAN instrumentation: table->record[0]
            fa331acefd6 improve ASAN instrumentation: mtr
            dc28b6d180a improve ASAN instrumentation: MEM_ROOT
            a966d422ca5 improve ASAN instrumentation: TRASH
            5e7593add40 add support for ASAN instrumentation
            6634f460a99 fix compilation with ASAN
            

            Works with safemalloc just fine.
            mem_heap_t and Pool are not instrumented.

            serg Sergei Golubchik added a comment - - edited Mostly already implemented with these commits: c3e25703c75 improve ASAN instrumentation: clang 03eb15933da improve ASAN instrumentation: InnoDB/XtraDB f2408e7e6a3 Free memory in unit tests. Makes ASAN happier. 36eb0b7a558 improve ASAN instrumentation: table->record[0] fa331acefd6 improve ASAN instrumentation: mtr dc28b6d180a improve ASAN instrumentation: MEM_ROOT a966d422ca5 improve ASAN instrumentation: TRASH 5e7593add40 add support for ASAN instrumentation 6634f460a99 fix compilation with ASAN Works with safemalloc just fine. mem_heap_t and Pool are not instrumented.
            marko Marko Mäkelä made changes -
            Field Original Value New Value
            Attachment MDEV-15030-Pool.patch [ 44944 ]
            marko Marko Mäkelä made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            marko Marko Mäkelä made changes -

            MDEV-15030-Pool.patch instruments the transaction pool and caught MDEV-15029 in MariaDB 10.2. Unfortunately some suppressions are necessary, because the transaction mutexes remain registered even for freed transactions. This patch is not applicable to earlier versions of MariaDB.

            I hope kevg can submit a revised pull request to instrument mem_heap_t in 5.5.

            marko Marko Mäkelä added a comment - MDEV-15030-Pool.patch instruments the transaction pool and caught MDEV-15029 in MariaDB 10.2. Unfortunately some suppressions are necessary, because the transaction mutexes remain registered even for freed transactions. This patch is not applicable to earlier versions of MariaDB. I hope kevg can submit a revised pull request to instrument mem_heap_t  in 5.5.

            As I can see from source code safemalloc does the following:
            1) checks for pointer alignment is 8
            2) checks for too big or too small values
            3) adds red zone on the left and on the right from memory chunk and catches overflow/underflow writes
            4) catches memory leaks on program exit

            1) is a standard requirement for malloc(), ASAN catches it
            2) ASAN catches here
            3) ASAN catches not only writes to red zone but also reads. And not only for heap memory but also for global/static variables and stack variables.
            4) LSAN does that and it's ON by default

            ASAN is superior to safemalloc. I suggest to disable safemalloc in ASAN build because it only brings slowdown which is not good if one want's to run the whole test suite.

            kevg Eugene Kosov (Inactive) added a comment - As I can see from source code safemalloc does the following: 1) checks for pointer alignment is 8 2) checks for too big or too small values 3) adds red zone on the left and on the right from memory chunk and catches overflow/underflow writes 4) catches memory leaks on program exit 1) is a standard requirement for malloc(), ASAN catches it 2) ASAN catches here 3) ASAN catches not only writes to red zone but also reads. And not only for heap memory but also for global/static variables and stack variables. 4) LSAN does that and it's ON by default ASAN is superior to safemalloc. I suggest to disable safemalloc in ASAN build because it only brings slowdown which is not good if one want's to run the whole test suite.
            kevg Eugene Kosov (Inactive) added a comment - - edited

            For ASAN TRASH_ALLOC() should be something like this

            #define TRASH_ALLOC(A,B) do { MEM_UNDEFINED(A,B); TRASH_FILL(A,B,0xA5); } while(0)
            

            Here we make memory region accessible and fill it with constant date for gdb user.

            But for valgrind it is

            #define TRASH_ALLOC(A,B) do { TRASH_FILL(A,B,0xA5); MEM_UNDEFINED(A,B); } while(0)
            

            Filling data and make it write-only.

            For ASAN TRASH_FREE() should be something like this:

            #define TRASH_FREE(A,B) do { MEM_NOACCESS(A,B); } while(0)
            

            No need to fill it with some data because every access will be caught regardless of this. Also, current code unpoisons, fills and poisons memory region. And this prevent ASAN from catching double free.

            MEM_UNDEFINED unpoisons memory, i.e. makes it correct. This is counter-intuitive, IMO. And that's why TRASH_ALLOC, TRASH_FREE macros ideally should be different for valgrind and ASAN.

            kevg Eugene Kosov (Inactive) added a comment - - edited For ASAN TRASH_ALLOC() should be something like this #define TRASH_ALLOC(A,B) do { MEM_UNDEFINED(A,B); TRASH_FILL(A,B,0xA5); } while(0) Here we make memory region accessible and fill it with constant date for gdb user. But for valgrind it is #define TRASH_ALLOC(A,B) do { TRASH_FILL(A,B,0xA5); MEM_UNDEFINED(A,B); } while(0) Filling data and make it write-only. For ASAN TRASH_FREE() should be something like this: #define TRASH_FREE(A,B) do { MEM_NOACCESS(A,B); } while(0) No need to fill it with some data because every access will be caught regardless of this. Also, current code unpoisons, fills and poisons memory region. And this prevent ASAN from catching double free. MEM_UNDEFINED unpoisons memory, i.e. makes it correct. This is counter-intuitive, IMO. And that's why TRASH_ALLOC, TRASH_FREE macros ideally should be different for valgrind and ASAN.

            Btw, the most efficient way for catching memory bugs would be disabling memory arenas completely. That would result in the best instrumentation out of the box. But sadly it's not possible with current code which never calls dummy free()/delete for arena allocations.

            kevg Eugene Kosov (Inactive) added a comment - Btw, the most efficient way for catching memory bugs would be disabling memory arenas completely. That would result in the best instrumentation out of the box. But sadly it's not possible with current code which never calls dummy free()/delete for arena allocations.

            features must be auto-disabled, if they don't work together. ASAN and safemalloc do.

            If you want ASAN without safemalloc, you do -DWITH_ASAN=ON -DWITH_SAFEMALLOC=OFF.

            TRASH_FILL() starts from MEM_UNDEFINED(), so TRASH_ALLOC is
            MEM_UNDEFINED(A,B); memset(A,B,0xA5); MEM_UNDEFINED(A,B); which is good both for ASAN and Valgrind.

            Same for TRASH_FREE.

            And if you want it to be just MEM_NOACCESS, you compile without debugging but with ASAN.

            serg Sergei Golubchik added a comment - features must be auto-disabled, if they don't work together. ASAN and safemalloc do. If you want ASAN without safemalloc, you do -DWITH_ASAN=ON -DWITH_SAFEMALLOC=OFF . TRASH_FILL() starts from MEM_UNDEFINED(), so TRASH_ALLOC is MEM_UNDEFINED(A,B); memset(A,B,0xA5); MEM_UNDEFINED(A,B); which is good both for ASAN and Valgrind. Same for TRASH_FREE. And if you want it to be just MEM_NOACCESS, you compile without debugging but with ASAN.

            Well, it's a bit coarse-grained.

            Here is the PR https://github.com/MariaDB/server/pull/569

            kevg Eugene Kosov (Inactive) added a comment - Well, it's a bit coarse-grained. Here is the PR https://github.com/MariaDB/server/pull/569
            marko Marko Mäkelä added a comment - - edited

            kevg, sorry, I did not realize that you had updated your contribution last night.
            I ended up doing something similar, just a bit more in order not to break UNIV_MEM_DEBUG (which admittedly is of little use) or Valgrind. With my changes, also with Valgrind, we will flag the address ranges unaccessible as early as possible.

            What remains to be done (once the code has been merged from 5.5 up to 10.2) is to add the address range poisoning to the transaction Pool. I believe that it should work with both ASAN and Valgrind.
            There will be some limitations for Valgrind, because apparently Valgrind discards the metadata on which bits are initialized when access to the address range is disabled. But I believe that we can address that by adding some UNIV_MEM_VALID to ‘initialize’ those fields that are supposed to remain initialized while the transaction objects are in the freed state.

            marko Marko Mäkelä added a comment - - edited kevg , sorry, I did not realize that you had updated your contribution last night. I ended up doing something similar, just a bit more in order not to break UNIV_MEM_DEBUG (which admittedly is of little use) or Valgrind. With my changes, also with Valgrind, we will flag the address ranges unaccessible as early as possible. What remains to be done (once the code has been merged from 5.5 up to 10.2) is to add the address range poisoning to the transaction Pool . I believe that it should work with both ASAN and Valgrind. There will be some limitations for Valgrind, because apparently Valgrind discards the metadata on which bits are initialized when access to the address range is disabled. But I believe that we can address that by adding some UNIV_MEM_VALID to ‘initialize’ those fields that are supposed to remain initialized while the transaction objects are in the freed state.

            I added poisoning to the Pool of trx_t, which was introduced via MySQL 5.7 to MariaDB 10.2.2. This revealed that innodb_monitor is accessing trx_t::mutex and trx_t::undo_mutex even for freed trx_t objects.
            With this instrumentation, I think that all dynamically managed memory is reasonably well poisoned.

            marko Marko Mäkelä added a comment - I added poisoning to the Pool of trx_t , which was introduced via MySQL 5.7 to MariaDB 10.2.2. This revealed that innodb_monitor is accessing trx_t::mutex and trx_t::undo_mutex even for freed trx_t  objects. With this instrumentation, I think that all dynamically managed memory is reasonably well poisoned.
            marko Marko Mäkelä made changes -
            issue.field.resolutiondate 2018-04-24 17:38:32.0 2018-04-24 17:38:32.068
            marko Marko Mäkelä made changes -
            Fix Version/s 10.2.15 [ 23006 ]
            Fix Version/s 10.3.7 [ 23005 ]
            Fix Version/s 10.3 [ 22126 ]
            Resolution Fixed [ 1 ]
            Status In Progress [ 3 ] Closed [ 6 ]
            marko Marko Mäkelä made changes -
            marko Marko Mäkelä made changes -
            marko Marko Mäkelä made changes -
            serg Sergei Golubchik made changes -
            Workflow MariaDB v3 [ 84997 ] MariaDB v4 [ 133443 ]

            People

              marko Marko Mäkelä
              svoj Sergey Vojtovich
              Votes:
              0 Vote for this issue
              Watchers:
              5 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.