Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix for resource deletion after tracer provider shutdown #911

Merged
merged 6 commits into from
Jul 22, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Jul 19, 2021

Fixes #904

Changes

Change in the order of declarations of tracer-context, instrumentation library, resources, and processor to ensure that resources and instrumentation library objects are destroyed after processor.
Adding test-case is a bit tricky ( would be doing it before merging the PR): but this is the batch-processor example output when tracer provider is deleted ( before and after changes):

Before change:

Shutting down and draining queue....
{
  name          : Span 1
  trace_id      : 738d6e1ac63df645a59696e9e8b3bc7b
  span_id       : 2bb3160e4738beae
  tracestate    :
  parent_span_id: 0000000000000000
  start         : 1626709154132876300
  duration      : 1700
  description   :
  span kind     : Internal
  status        : Unset
  attributes    :
  events        :
  links         :
  resources     :
  instr-lib     : �"VُY7�`Y"V`0"V."V."VA0@"V�"V>"V>"VA]�f(JY¤C�`Y"V`0"V."VSY;��`?"V�Span 1?"V?"V�?(4"V05"VA�"Vp+"VA�>"]�5�@'@'?ײE
�`Y"V`0"V."VTY;�@"V�Span A"VHA"V�?(4"V05"VA�p@"]�$Xj[W=}4N�`Y"V`0"V."V'UY;�� B"V�Span 3HB"V

After change:

Shutting down and draining queue....
{
  name          : Span 10
  trace_id      : faa0a602c715e0019e0c9da8cac147bc
  span_id       : 18cd2ae1bc8d2724
  tracestate    :
  parent_span_id: 0000000000000000
  start         : 1626708952913928200
  duration      : 1600
  description   :
  span kind     : Internal
  status        : Unset
  attributes    :
  events        :
  links         :
  resources     :
        service.name: unknown_service
        telemetry.sdk.version: 1.0.0-rc3
        telemetry.sdk.name: opentelemetry
        version: 1
        telemetry.sdk.language: cpp
        service: test_service
  instr-lib     : foo_library
}

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team July 19, 2021 15:52
@codecov
Copy link

codecov bot commented Jul 19, 2021

Codecov Report

Merging #911 (788076d) into main (afcb08e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #911   +/-   ##
=======================================
  Coverage   95.44%   95.44%           
=======================================
  Files         158      158           
  Lines        6749     6749           
=======================================
  Hits         6441     6441           
  Misses        308      308           
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/trace/tracer.h 100.00% <ø> (ø)
sdk/src/trace/tracer.cc 87.50% <100.00%> (ø)
sdk/src/trace/tracer_context.cc 81.82% <100.00%> (ø)

Copy link
Contributor

@seemk seemk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stumbled upon this, tested this PR and can confirm this fixes the use after free.

Thanks!

@lalitb
Copy link
Member Author

lalitb commented Jul 20, 2021

@open-telemetry/cpp-approvers need help reviewing this. Its small change to reorder declaration for the fix.

@lalitb lalitb merged commit 083cf02 into open-telemetry:main Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BatchSpanProcessor accesses deleted resources after tracer destruction
3 participants