search

grobian / carbon-c-relay

Enhanced C implementation of Carbon relay, aggregator and rewriter
Apache License 2.0
380 stars 107 forks source link

Use after free during the shutdown #200

Closed Civil closed 8 years ago

Civil commented 8 years ago

This luckily doesn't cause any problems at this time, but might cause segfault during the shutdown in rare cases:

[2016-07-01 12:00:57] shutting down...
[2016-07-01 12:00:57] closed listeners for port 2003
[2016-07-01 12:00:57] stopped collector
[2016-07-01 12:00:58] stopped aggregator
=================================================================
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==ERROR: AddressSanitizer: heap-use-after-free on address 0x60600000ebb8 at pc 0x00000053dba3 bp 0x7fb5dbae8f70 sp 0x7fb5dbae8f68
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
==20613==AddressSanitizer: while reporting a bug found another one. Ignoring.
READ of size 8 at 0x60600000ebb8 thread T8
    #0 0x53dba2 in aggregator_putmetric /usr/src/carbon-c-relay/aggregator.c:205:13
    #1 0x52bb9e in router_route_intern /usr/src/carbon-c-relay/router.c:2899:7
    #2 0x529b5b in router_route /usr/src/carbon-c-relay/router.c:2980:8
    #3 0x503ed9 in dispatch_connection /usr/src/carbon-c-relay/dispatcher.c:404:25
    #4 0x502b97 in dispatch_runner /usr/src/carbon-c-relay/dispatcher.c:585:13
    #5 0x7fb5e413e493 in start_thread (/lib64/libpthread.so.0+0x7493)
    #6 0x7fb5e3a645dc in clone (/lib64/libc.so.6+0xe95dc)

0x60600000ebb8 is located 24 bytes inside of 64-byte region [0x60600000eba0,0x60600000ebe0)
freed by thread T23 here:
    #0 0x4c1210 in free (/usr/src/carbon-c-relay/relay+0x4c1210)
    #1 0x5422a9 in aggregator_expire /usr/src/carbon-c-relay/aggregator.c:609:3
    #2 0x7fb5e413e493 in start_thread (/lib64/libpthread.so.0+0x7493)

previously allocated by thread T0 here:
    #0 0x4c1518 in __interceptor_malloc (/usr/src/carbon-c-relay/relay+0x4c1518)
    #1 0x53c9f3 in aggregator_new /usr/src/carbon-c-relay/aggregator.c:53:20
    #2 0x5147f8 in router_readconfig /usr/src/carbon-c-relay/router.c:1312:29
    #3 0x4f4525 in main /usr/src/carbon-c-relay/relay.c:630:13
    #4 0x7fb5e399b61f in __libc_start_main (/lib64/libc.so.6+0x2061f)

Thread T8 created by T0 here:
    #0 0x42c129 in __interceptor_pthread_create (/usr/src/carbon-c-relay/relay+0x42c129)
    #1 0x5014b9 in dispatch_new /usr/src/carbon-c-relay/dispatcher.c:625:6
    #2 0x501547 in dispatch_new_connection /usr/src/carbon-c-relay/dispatcher.c:654:9
    #3 0x4f5032 in main /usr/src/carbon-c-relay/relay.c:722:21
    #4 0x7fb5e399b61f in __libc_start_main (/lib64/libc.so.6+0x2061f)

Thread T23 created by T0 here:
    #0 0x42c129 in __interceptor_pthread_create (/usr/src/carbon-c-relay/relay+0x42c129)
    #1 0x53facf in aggregator_start /usr/src/carbon-c-relay/aggregator.c:655:6
    #2 0x4f52c5 in main /usr/src/carbon-c-relay/relay.c:751:8
    #3 0x7fb5e399b61f in __libc_start_main (/lib64/libc.so.6+0x2061f)

SUMMARY: AddressSanitizer: heap-use-after-free /usr/src/carbon-c-relay/aggregator.c:205:13 in aggregator_putmetric
Shadow bytes around the buggy address:
  0x0c0c7fff9d20: fa fa fa fa fa fa fa fa fa fa fa fa fd fd fd fd
  0x0c0c7fff9d30: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0c7fff9d40: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c0c7fff9d50: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
  0x0c0c7fff9d60: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
=>0x0c0c7fff9d70: fa fa fa fa fd fd fd[fd]fd fd fd fd fa fa fa fa
  0x0c0c7fff9d80: 00 00 00 00 00 00 00 00 fa fa fa fa fd fd fd fd
  0x0c0c7fff9d90: fd fd fd fd fa fa fa fa 00 00 00 00 00 00 00 fa
  0x0c0c7fff9da0: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0c7fff9db0: 00 00 00 00 00 00 02 fa fa fa fa fa 00 00 00 00
  0x0c0c7fff9dc0: 00 00 00 01 fa fa fa fa 00 00 00 00 00 00 00 00
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
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  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
==20613==ABORTING

I also kindly suggesting you to add some simple test for aggregator and relay with software compiled with latest clang and ASAN + MSAN as a must-pass test before any release.

(this is copy of previous report, so it might be shifted now by approx 10 lines).

grobian commented 8 years ago

analysis: dispatchers are still running, while aggregators are freed, this is on purpose, since aggregations need to be routed on shutdown. aggregator_putmetric should likely discard any updates after shutdown to avoid this situation.

grobian commented 8 years ago

should be fixed by 0980c5e