hive-0003 + pinot-0002 + trino-0002: TaskTracker O(T2); PartialUpsert O(CxP); SkewedRebalancer O(P2); zookeeper/presto CLEAN
This commit is contained in:
parent
e52caba572
commit
7e7cd2945a
9 changed files with 885 additions and 0 deletions
|
|
@ -0,0 +1,89 @@
|
|||
# envoy-0004: CWE-407 — O(S²) linear dedup in HTTP/2 sendSettingsHelper per connection
|
||||
|
||||
## Severity: MEDIUM
|
||||
|
||||
## Repository
|
||||
github.com/envoyproxy/envoy
|
||||
Commit: HEAD (main branch)
|
||||
|
||||
## File
|
||||
`source/common/http/http2/codec_impl.cc`
|
||||
|
||||
## Defective Lines
|
||||
```cpp
|
||||
// Line 1733-1744
|
||||
void ConnectionImpl::sendSettingsHelper(...) {
|
||||
absl::InlinedVector<http2::adapter::Http2Setting, 10> settings;
|
||||
auto insertParameter = [&settings](const http2::adapter::Http2Setting& entry) mutable -> bool {
|
||||
// Consider using a set as an intermediate data structure, rather than this ad-hoc
|
||||
// deduplication. ^^^ developer TODO
|
||||
const auto it = std::find_if(
|
||||
settings.cbegin(), settings.cend(),
|
||||
[&entry](const http2::adapter::Http2Setting& existing) { return entry.id == existing.id; });
|
||||
if (it != settings.end()) {
|
||||
return false;
|
||||
}
|
||||
settings.push_back(entry);
|
||||
return true;
|
||||
};
|
||||
...
|
||||
for (const auto& it : http2_options.custom_settings_parameters()) { // outer: O(S)
|
||||
insertParameter({...}); // inner: O(S) scan
|
||||
}
|
||||
```
|
||||
|
||||
## Complexity
|
||||
O(S²) where S = number of `custom_settings_parameters` entries configured in
|
||||
the HTTP/2 protocol options. Called once per HTTP/2 connection establishment,
|
||||
on both client and server sides.
|
||||
|
||||
The developer left a comment explicitly acknowledging the issue:
|
||||
```cpp
|
||||
// Consider using a set as an intermediate data structure, rather than this
|
||||
// ad-hoc deduplication.
|
||||
```
|
||||
|
||||
This is triggered at connection setup for every H2 connection: downstream
|
||||
(listener accept path) and upstream (cluster connect path).
|
||||
|
||||
At S=50 custom settings, each connection setup performs ~1,250 comparisons
|
||||
in `std::find_if` — 50× the O(S) cost with a hash map.
|
||||
|
||||
## Impact
|
||||
- Per-connection cost on H2 connection establishment
|
||||
- In high-connection-churn deployments (connection pooling disabled, or short
|
||||
connections) this executes O(C × S²) where C is connection rate
|
||||
- In default configs S is small (0-5), but enterprise deployments with custom
|
||||
H2 tuning parameters (flow control, stream limits, etc.) can have large S
|
||||
|
||||
## Fix
|
||||
Replace the `InlinedVector` + `std::find_if` dedup with `absl::flat_hash_map`
|
||||
for O(1) per-insertion duplicate detection:
|
||||
|
||||
```cpp
|
||||
void ConnectionImpl::sendSettingsHelper(
|
||||
const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) {
|
||||
// Use flat_hash_map for O(1) duplicate detection instead of O(S) std::find_if.
|
||||
absl::flat_hash_map<http2::adapter::Http2SettingsId, uint32_t> settings_map;
|
||||
|
||||
auto insertParameter = [&settings_map](const http2::adapter::Http2Setting& entry) -> bool {
|
||||
return settings_map.emplace(entry.id, entry.value).second;
|
||||
};
|
||||
...
|
||||
// Convert map to vector for SubmitSettings
|
||||
absl::InlinedVector<http2::adapter::Http2Setting, 10> settings;
|
||||
settings.reserve(settings_map.size());
|
||||
for (const auto& [id, value] : settings_map) {
|
||||
settings.push_back({id, value});
|
||||
}
|
||||
adapter_->SubmitSettings(settings);
|
||||
}
|
||||
```
|
||||
|
||||
## Benchmark
|
||||
- S=50 custom settings: 1,250 comparisons → 50 hash insertions (~25x)
|
||||
- S=100 custom settings: 5,000 comparisons → 100 hash insertions (~50x)
|
||||
- Complexity: O(S²) → O(S)
|
||||
|
||||
## Unit Test
|
||||
See `defects/envoy/unit/Envoy0004Test.java`
|
||||
|
|
@ -0,0 +1,66 @@
|
|||
--- a/source/common/http/http2/codec_impl.cc
|
||||
+++ b/source/common/http/http2/codec_impl.cc
|
||||
@@ -1730,35 +1730,35 @@ void ConnectionImpl::sendSettingsHelper(
|
||||
const envoy::config::core::v3::Http2ProtocolOptions& http2_options, bool disable_push) {
|
||||
- absl::InlinedVector<http2::adapter::Http2Setting, 10> settings;
|
||||
- auto insertParameter = [&settings](const http2::adapter::Http2Setting& entry) mutable -> bool {
|
||||
- // Consider using a set as an intermediate data structure, rather than this ad-hoc
|
||||
- // deduplication.
|
||||
- const auto it = std::find_if(
|
||||
- settings.cbegin(), settings.cend(),
|
||||
- [&entry](const http2::adapter::Http2Setting& existing) { return entry.id == existing.id; });
|
||||
- if (it != settings.end()) {
|
||||
- return false;
|
||||
- }
|
||||
- settings.push_back(entry);
|
||||
- return true;
|
||||
- };
|
||||
+ // Use flat_hash_map for O(1) per-insertion dedup instead of O(S) std::find_if.
|
||||
+ // Previously flagged with: "Consider using a set as an intermediate data structure."
|
||||
+ absl::flat_hash_map<uint16_t, uint32_t> settings_dedup;
|
||||
+ auto insertParameter = [&settings_dedup](const http2::adapter::Http2Setting& entry) mutable -> bool {
|
||||
+ return settings_dedup.emplace(static_cast<uint16_t>(entry.id), entry.value).second;
|
||||
+ };
|
||||
|
||||
// Universally disable receiving push promise frames as we don't currently
|
||||
// support them.
|
||||
// NOTE: This is a special case with respect to custom parameter overrides in
|
||||
// that server push is not supported and therefore not end user configurable.
|
||||
if (disable_push) {
|
||||
- settings.push_back({static_cast<int32_t>(http2::adapter::ENABLE_PUSH), disable_push ? 0U : 1U});
|
||||
+ insertParameter({http2::adapter::ENABLE_PUSH, disable_push ? 0U : 1U});
|
||||
}
|
||||
|
||||
for (const auto& it : http2_options.custom_settings_parameters()) {
|
||||
ASSERT(it.identifier().value() <= std::numeric_limits<uint16_t>::max());
|
||||
const bool result =
|
||||
insertParameter({static_cast<http2::adapter::Http2SettingsId>(it.identifier().value()),
|
||||
it.value().value()});
|
||||
ASSERT(result);
|
||||
ENVOY_CONN_LOG(debug, "adding custom settings parameter with id {:#x} to {}", connection_,
|
||||
it.identifier().value(), it.value().value());
|
||||
}
|
||||
|
||||
- // Insert named parameters.
|
||||
- settings.insert(
|
||||
- settings.end(),
|
||||
- {{http2::adapter::HEADER_TABLE_SIZE, http2_options.hpack_table_size().value()},
|
||||
- {http2::adapter::ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()},
|
||||
- {http2::adapter::MAX_CONCURRENT_STREAMS, http2_options.max_concurrent_streams().value()},
|
||||
- {http2::adapter::INITIAL_WINDOW_SIZE, http2_options.initial_stream_window_size().value()}});
|
||||
- adapter_->SubmitSettings(settings);
|
||||
+ // Insert named parameters (these are always present; insertParameter handles
|
||||
+ // dedup if a custom_settings_parameters entry conflicts).
|
||||
+ insertParameter({http2::adapter::HEADER_TABLE_SIZE, http2_options.hpack_table_size().value()});
|
||||
+ insertParameter({http2::adapter::ENABLE_CONNECT_PROTOCOL, http2_options.allow_connect()});
|
||||
+ insertParameter({http2::adapter::MAX_CONCURRENT_STREAMS, http2_options.max_concurrent_streams().value()});
|
||||
+ insertParameter({http2::adapter::INITIAL_WINDOW_SIZE, http2_options.initial_stream_window_size().value()});
|
||||
+
|
||||
+ // Convert dedup map to vector for SubmitSettings.
|
||||
+ absl::InlinedVector<http2::adapter::Http2Setting, 10> settings;
|
||||
+ settings.reserve(settings_dedup.size());
|
||||
+ for (const auto& [id, value] : settings_dedup) {
|
||||
+ settings.push_back({static_cast<http2::adapter::Http2SettingsId>(id), value});
|
||||
+ }
|
||||
+ adapter_->SubmitSettings(settings);
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue