matcher: fix UB bug caused by dereferencing a bad optional#14271
matcher: fix UB bug caused by dereferencing a bad optional#14271snowp merged 1 commit intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Snow Pettersen <snowp@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
This makes sense to me but how does this not fail every time on every build? I'm confused.
/wait-any
|
I'm guessing it has something to do with the difference between libstdc++ and libc++: the compile_time_options target is the only build that runs these tests with libstdc++. Best guess is that the implementation of std::optional (which absl::optional delegates to when present iiuc) is different between enough between the two that the behavior when invoking UB is different. They both seem to rely on Not sure if it's worth it to dig into this to understand the specifics, lmk |
It's not a huge deal, but I thought the standard says that when an optional does not contain a value it's supposed to throw a bad optional exception. How can this be non-deterministic one way or the other? Something seems off here. Is this test not deterministic? |
|
https://en.cppreference.com/w/cpp/utility/optional/operator* is UB when the optional does not contain a value, while https://en.cppreference.com/w/cpp/utility/optional/value throws an exception when the optional does not contain a value. This was surprising to me, I thought they were identical until I looked into it for this issue. Seems possible that the result can be non-deterministic if we're invoking UB? I guess you could expect subsequent test runs to have the same data left on the stack if we're just dealing with an uninitialized variable, so maybe that's not a great explanation? |
|
Sigh C++. Pretty annoying that they have different behavior but I guess it's similar to array indexing vs. at() in vector, etc. OK makes sense for some definition of making sense. |
* master: (41 commits) event: Remove a source of non-determinism by always running deferred deletion before post callbacks (envoyproxy#14293) Fix TSAN bug in integration test (envoyproxy#14327) tracing: Add hostname to Zipkin config. (envoyproxy#14186) (envoyproxy#14187) [conn_pool] fix use after free in H/1 connection pool (envoyproxy#14220) lua: update deprecated lua_open to luaL_newstate (envoyproxy#14297) extension: use bool_flag to control extension link (envoyproxy#14240) stats: Factor out creation of cluster-stats StatNames from creation of the stats, to save CPU during xDS updates (envoyproxy#14028) test: add scaled timer integration test (envoyproxy#14290) [Win32 Signals] Add term and ctrl-c signal handlers (envoyproxy#13954) config: v2 transport API fatal-by-default. (envoyproxy#14223) matcher: fix UB bug caused by dereferencing a bad optional (envoyproxy#14271) test: putting fake upstream config in a struct (envoyproxy#14266) wasm: use Bazel rules from Proxy-Wasm Rust SDK. (envoyproxy#14292) docs: fix typo (envoyproxy#14237) dependencies: allowlist CVE-2018-21270 to prevent false positives. (envoyproxy#14294) typo in redis doc (envoyproxy#14248) access_loggers: removed redundant dep (envoyproxy#14274) fix http2 flaky test (envoyproxy#14261) test: disable flaky xds_integration_test. (envoyproxy#14287) http: add functionality to configure kill header in KillRequest proto (envoyproxy#14288) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
2 participants
This fixes a flaky test on master.
Signed-off-by: Snow Pettersen snowp@lyft.com
Risk Level: Low, fixes unused code
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a