Skip to content

Commit

Permalink
[trace-reader] Avoid UBSAN offsetting nullptr
Browse files Browse the repository at this point in the history
```
../../zircon/system/ulib/trace-reader/reader.cc:766:16: runtime error: applying non-zero offset 8 to null pointer
   #0    0x0000425cf320f4bc in trace::Chunk::ReadString(trace::Chunk*, size_t, std::__2::string_view*) ../../zircon/system/ulib/trace-reader/reader.cc:766 <<application>>+0x1044bc
   #1.2  0x00004377ab0103c0 in ubsan_GetStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:43 <libclang_rt.asan.so>+0x363c0
   #1.1  0x00004377ab0103c0 in MaybePrintStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:53 <libclang_rt.asan.so>+0x363c0
   #1    0x00004377ab0103c0 in ~ScopedReport() compiler-rt/lib/ubsan/ubsan_diag.cpp:389 <libclang_rt.asan.so>+0x363c0
   #2    0x00004377ab0137c8 in handlePointerOverflowImpl() compiler-rt/lib/ubsan/ubsan_handlers.cpp:809 <libclang_rt.asan.so>+0x397c8
   #3    0x00004377ab01343c in compiler-rt/lib/ubsan/ubsan_handlers.cpp:815 <libclang_rt.asan.so>+0x3943c
   #4    0x0000425cf320f4bc in trace::Chunk::ReadString(trace::Chunk*, size_t, std::__2::string_view*) ../../zircon/system/ulib/trace-reader/reader.cc:766 <<application>>+0x1044bc
   #5    0x0000425cf316b31c in trace::$anon::TraceReader_EmptyChunk_Class::TestBody(trace::$anon::TraceReader_EmptyChunk_Class*) ../../zircon/system/ulib/trace-reader/test/reader_tests.cc:39 <<application>>+0x6031c
   #6    0x0000425cf322b564 in zxtest::Test::Run(zxtest::Test*) ../../zircon/system/ulib/zxtest/test.cc:18 <<application>>+0x120564
   #7    0x0000425cf3229d6c in zxtest::TestCase::Run(zxtest::TestCase*, zxtest::LifecycleObserver*, zxtest::internal::TestDriver*) ../../zircon/system/ulib/zxtest/test-case.cc:106 <<application>>+0x11ed6c
   #8    0x0000425cf321e8f0 in zxtest::Runner::Run(zxtest::Runner*, const zxtest::Runner::Options&) ../../zircon/system/ulib/zxtest/runner.cc:121 <<application>>+0x1138f0
   #9    0x0000425cf32204e0 in zxtest::RunAllTests(int, char**) ../../zircon/system/ulib/zxtest/runner.cc:217 <<application>>+0x1154e0
   #10   0x0000425cf322b648 in main(int, char**) ../../zircon/system/ulib/zxtest/zxtest-main.cc:14 <<application>>+0x120648
   #11   0x0000827600fbc654 in start_main(const start_params*) ../../zircon/third_party/ulib/musl/src/env/__libc_start_main.c:139 <libc.so>+0xcc654
   #12   0x0000827600fbcf24 in __libc_start_main(zx_handle_t, int (*)(int, char**, char**)) ../../zircon/third_party/ulib/musl/src/env/__libc_start_main.c:256 <libc.so>+0xccf24
   #13   0x0000425cf320a510 in _start(zx_handle_t) ../../zircon/system/ulib/c/Scrt1.cc:7 <<application>>+0xff510
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior    #0    0x0000000000000000 is not covered by any module
```

It is impossible to use empty chunks without triggering UB. Delete the
default constructor and change surrounding APIs to avoid the need.

Fixed: 41892
Change-Id: Ia340d4e99d4f709d4f58dd85a7bc241918115e7e
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/574302
Commit-Queue: Auto-Submit <[email protected]>
Fuchsia-Auto-Submit: Tamir Duberstein <[email protected]>
Reviewed-by: Fadi Meawad <[email protected]>
  • Loading branch information
tamird authored and CQ Bot committed Aug 27, 2021
1 parent 81e24af commit e5baece
Show file tree
Hide file tree
Showing 4 changed files with 348 additions and 254 deletions.
4 changes: 0 additions & 4 deletions zircon/system/ulib/trace-reader/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ zx_library("trace-reader") {
]
deps = [ "//zircon/public/lib/fbl" ]

# TODO(fxbug.dev/41892): UBSan has found an instance of undefined behavior in this target.
# Disable UBSan for this target temporarily until it is migrated into CI/CQ.
configs += [ "//build/config:temporarily_disable_ubsan_do_not_use" ]

# TODO(fxbug.dev/58162): delete the below and fix compiler warnings
configs += [ "//build/config:Wno-conversion" ]
}
Expand Down
22 changes: 11 additions & 11 deletions zircon/system/ulib/trace-reader/include/trace-reader/reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <zircon/assert.h>

#include <memory>
#include <optional>
#include <string_view>
#include <utility>

Expand Down Expand Up @@ -47,7 +48,7 @@ class TraceReader {
// Reads as many records as possible from the chunk, invoking the
// record consumer for each one. Returns true if the stream could possibly
// contain more records if the chunk were extended with new data.
// Returns false if the trace stream is unrecoverably corrupt and no
// Returns false if the trace stream is irrecoverably corrupt and no
// further decoding is possible. May be called repeatedly with new
// chunks as they become available to resume decoding.
bool ReadRecords(Chunk& chunk);
Expand Down Expand Up @@ -86,7 +87,7 @@ class TraceReader {

void SetCurrentProvider(ProviderId id);
void RegisterProvider(ProviderId id, fbl::String name);
void RegisterString(trace_string_index_t index, fbl::String string);
void RegisterString(trace_string_index_t index, const fbl::String& string);
void RegisterThread(trace_thread_index_t index, const ProcessThread& process_thread);

bool DecodeStringRef(Chunk& chunk, trace_encoded_string_ref_t string_ref,
Expand Down Expand Up @@ -127,7 +128,7 @@ class TraceReader {
ProviderId id;
fbl::String name;

// TODO(fxbug.dev/30999): It would be more efficient to use something like
// TODO(https://fxbug.dev/30999): It would be more efficient to use something like
// std::unordered_map<> here. In particular, the table entries are
// small enough that it doesn't make sense to heap allocate them
// individually.
Expand All @@ -149,8 +150,7 @@ class TraceReader {
// region of memory. The main use-case of this class is input to |TraceReader|.
class Chunk final {
public:
Chunk();
explicit Chunk(const uint64_t* begin, size_t num_words);
Chunk(const uint64_t* begin, size_t num_words);

uint64_t current_byte_offset() const {
return (reinterpret_cast<const uint8_t*>(current_) - reinterpret_cast<const uint8_t*>(begin_));
Expand All @@ -160,12 +160,12 @@ class Chunk final {
// Reads from the chunk, maintaining proper alignment.
// Returns true on success, false if the chunk has insufficient remaining
// words to satisfy the request.
bool ReadUint64(uint64_t* out_value);
bool ReadInt64(int64_t* out_value);
bool ReadDouble(double* out_value);
bool ReadString(size_t length, std::string_view* out_string);
bool ReadChunk(size_t num_words, Chunk* out_chunk);
bool ReadInPlace(size_t num_words, const void** out_ptr);
std::optional<uint64_t> ReadUint64();
std::optional<int64_t> ReadInt64();
std::optional<double> ReadDouble();
std::optional<std::string_view> ReadString(size_t length);
std::optional<Chunk> ReadChunk(size_t num_words);
std::optional<void const*> ReadInPlace(size_t num_words);

private:
const uint64_t* begin_;
Expand Down
Loading

0 comments on commit e5baece

Please sign in to comment.