Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix CIGAR reallocation with --eqx #175

Merged
merged 1 commit into from
Jun 19, 2018
Merged

Conversation

amwenger
Copy link

@amwenger amwenger commented Jun 4, 2018

Fix the logic that calculates the number of CIGAR entries when
match "M" entries are expanded into "=" and "X". The number
of entries depends not on the number of mismatches but rather
on the number of transitions between "=" to "X".

A test case that segfaults with the prior implementation is available
at https://gist.github.com/amwenger/b1e56ce4d9895d8f0c13707b6916800b.

# segfaults on Ubuntu 14
minimap2 --eqx -a eqxref.fa eqxread.fa 1>/dev/null

cc: @armintoepfer

Fix the logic that calculates the number of CIGAR entries when
match "M" entries are expanded into "=" and "X".  The number
of entries depends not on the number of mismatches but rather
on the number of transitions between "=" to "X".
@armintoepfer
Copy link
Contributor

armintoepfer commented Jun 4, 2018

fwiw, I ran master through clang ASAN:

WRITE of size 4 at 0x625000022900 thread T9
    #0 0x10f81663e in mm_update_cigar_eqx align.c:282
    #1 0x10f7f9272 in mm_align1 align.c:746
    #2 0x10f7ee0b6 in mm_align_skeleton align.c:860
    #3 0x10f8449e6 in align_regs map.c:253
    #4 0x10f83c40c in mm_map_frag map.c:343
    #5 0x10f84f92e in worker_for map.c:424
    #6 0x10f7a1a34 in ktf_worker kthread.c:47
    #7 0x7fff67e05660 in _pthread_body (libsystem_pthread.dylib:x86_64+0x3660)
    #8 0x7fff67e0550c in _pthread_start (libsystem_pthread.dylib:x86_64+0x350c)
    #9 0x7fff67e04bf8 in thread_start (libsystem_pthread.dylib:x86_64+0x2bf8)

0x625000022900 is located 0 bytes to the right of 8192-byte region [0x625000020900,0x625000022900)
allocated by thread T9 here:
    #0 0x10f9c3367 in wrap_calloc (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x57367)
    #1 0x10f814bfa in mm_update_cigar_eqx align.c:254
    #2 0x10f7f9272 in mm_align1 align.c:746
    #3 0x10f7ee0b6 in mm_align_skeleton align.c:860
    #4 0x10f8449e6 in align_regs map.c:253
    #5 0x10f83c40c in mm_map_frag map.c:343
    #6 0x10f84f92e in worker_for map.c:424
    #7 0x10f7a1a34 in ktf_worker kthread.c:47
    #8 0x7fff67e05660 in _pthread_body (libsystem_pthread.dylib:x86_64+0x3660)
    #9 0x7fff67e0550c in _pthread_start (libsystem_pthread.dylib:x86_64+0x350c)
    #10 0x7fff67e04bf8 in thread_start (libsystem_pthread.dylib:x86_64+0x2bf8)

Thread T9 created by T7 here:
    #0 0x10f9bae1d in wrap_pthread_create (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x4ee1d)
    #1 0x10f7a1810 in kt_for kthread.c:65
    #2 0x10f845f77 in worker_pipeline map.c:472
    #3 0x10f7a33aa in ktp_worker kthread.c:118
    #4 0x7fff67e05660 in _pthread_body (libsystem_pthread.dylib:x86_64+0x3660)
    #5 0x7fff67e0550c in _pthread_start (libsystem_pthread.dylib:x86_64+0x350c)
    #6 0x7fff67e04bf8 in thread_start (libsystem_pthread.dylib:x86_64+0x2bf8)

Thread T7 created by T0 here:
    #0 0x10f9bae1d in wrap_pthread_create (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x4ee1d)
    #1 0x10f7a2936 in kt_pipeline kthread.c:153
    #2 0x10f8457b6 in mm_map_file_frag map.c:540
    #3 0x10f84ab88 in mm_map_file map.c:550
    #4 0x10f79b64d in main main.c:339
    #5 0x7fff67aed014 in start (libdyld.dylib:x86_64+0x1014)

SUMMARY: AddressSanitizer: heap-buffer-overflow align.c:282 in mm_update_cigar_eqx
Shadow bytes around the buggy address:
  0x1c4a000044d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c4a000044e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c4a000044f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c4a00004500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1c4a00004510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1c4a00004520:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c4a00004530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c4a00004540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c4a00004550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c4a00004560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c4a00004570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==37423==ABORTING```

The PR fixes the overflow

@lh3 lh3 merged commit 3d3bcc2 into lh3:master Jun 19, 2018
@lh3
Copy link
Owner

lh3 commented Jun 19, 2018

Thanks. I thought 2*n_diff-n_M should be sufficient, but clearly I was wrong.

@lh3 lh3 added the bug label Jun 20, 2018
@lh3 lh3 added this to the 2.11 milestone Jun 24, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants