open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
850 stars 403 forks source link

UndefinedBehaviorSanitizer detects issues in `nostd::shared_ptr` #1337

Open bjosv opened 2 years ago

bjosv commented 2 years ago

When using nostd::shared_ptr and building with an undefined behavior detector there are indications of vptr issues. It's an use of an object whose vptr indicates that it is of the wrong dynamic type. The issue can be triggered by the existing tests as well:

Steps to reproduce

# cmake -DCMAKE_CXX_FLAGS='-fsanitize=undefined -fno-omit-frame-pointer -fno-sanitize-recover=all' .. 
# make all test
    The following tests FAILED:
     70 - nostd.SharedPtrTest.MoveConstructionFromDifferentType (Failed)
     78 - nostd.SharedPtrTest.Sort (Failed)
     90 - trace.Provider.GetTracerProviderDefault (Failed)
     91 - trace.Provider.SetTracerProvider (Failed)

# ./api/test/nostd/shared_ptr_test
...
[ RUN      ] SharedPtrTest.MoveConstructionFromDifferentType
/home/bjorn/git/opentelemetry-cpp/api/include/opentelemetry/nostd/shared_ptr.h:126:55: runtime error: member call on address 0x7ffc5b589240 which does not point to an object of type 'shared_ptr_wrapper'
0x7ffc5b589240: note: object is of type 'opentelemetry::v1::nostd::shared_ptr<int>::shared_ptr_wrapper'
 94 55 00 00  28 ee 4b 4d 94 55 00 00  c0 e3 a4 4d 94 55 00 00  e0 e3 a4 4d 94 55 00 00  15 15 47 4d
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'opentelemetry::v1::nostd::shared_ptr<int>::shared_ptr_wrapper'

It seems that the shared_ptr_wrapper is of type nostd::shared_ptr<T>::shared_ptr_wrapper while other.wrapper() is a nostd::shared_ptr<U>.

This gives that shared_ptr_wrapper::MoveTo will store a nostd::shared_ptr<U>::shared_ptr_wrapper inside a nostd::shared_ptr<T>::buffer_, but it will access it as a nostd::shared_ptr<T>::shared_ptr_wrapper which causes this UBSAN error.

I have not found any discussions or issues regarding this, but the question is if it's a know issue or as design?

Additional context The specific check that triggers is -fsanitize=vptr which is part of the general group -fsanitize=undefined

lalitb commented 2 years ago

thanks for reporting. This was not discussed earlier, so need to look into it. Also, if this is false-positive (i.e. reported by UBSan `without any real undefined behavior at run-time), we may just need to document this.

github-actions[bot] commented 2 years ago

This issue was marked as stale due to lack of activity.

lalitb commented 2 years ago

Reopening it, for now, we can discuss it in the next community meeting how to handle this.