[test] fix fuzz tests that might crash on duplicate settings params #10779
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
|
Actually: Probably this test utility should not hold settings parmas in an unordered set. nghttp2 should handle (with an invalid argument failure) duplicate settings ID, so there's no need to prevent testing that codepath. Edit: The method I found for nghttp2 that would return an error is a test utility I think? I'm really not sure (https://nghttp2.org/documentation/nghttp2_pack_settings_payload.html). I'm going off spec now. |
Signed-off-by: Asra Ali <asraa@google.com>
| if (!result.second) { | ||
| ENVOY_LOG_MISC(debug, "Duplicated settings parameter {} with value {}", | ||
| settings_frame.iv[i].settings_id, settings_frame.iv[i].value); | ||
| settings_.erase(result.first); |
There was a problem hiding this comment.
It seems preferable to use a map instead now that uniqueness checks are not required.
There was a problem hiding this comment.
It depends on what the intent of this helper was. If it was to detect erroneous accidental double settings in tests, then the original was best.
It's a bit strange that we do a set of nghttp2_settings_entry instead of a map<settings_id, value> and fail when setting_id is duplicated. I can see us doing the nghttp2_settings_entry if we're trying to avoid errorneously sending the exact same setting_id, value pair multiple times.
There was a problem hiding this comment.
The intent was to simply provide a helper to fetch SETTINGS parameters received from a peer. The use of the set was based on my misunderstanding of the RFC; the goal was to be very explicit about the uniqueness requirement through the type used.
There was a problem hiding this comment.
Following up on Antonio's point about accidentally sending duplicate pairs, we should add a test that validates that condition does not happen, but this utility should be generic enough to support both the existing unit tests, fuzz tests and to-be-written duplicate parameter tests.
There was a problem hiding this comment.
Changed to an unordered-map. this isn't relevant to sendSettings, which fwiw parses configuration settings for your codecs. (And we can impose restrictions like do not duplicate settings in your config). I can write a test that sends a settingsframe containing duplicate settings in a frame (this would have always worked against a real envoy, since this is just test utility)
| settings_frame.iv[i].settings_id, settings_frame.iv[i].value); | ||
| settings_.erase(result.first); | ||
| // Guaranteed success here. | ||
| ASSERT(settings_.insert(settings_frame.iv[i]).second); |
There was a problem hiding this comment.
The body of ASSERTs can't be side effecting, since they disappear in opt build (and we do run tests on opt builds)..
There was a problem hiding this comment.
Oh, in hindsight that was pretty silly. Thank you!
Signed-off-by: Asra Ali <asraa@google.com>
…nvoyproxy#10779) Signed-off-by: Asra Ali <asraa@google.com> Signed-off-by: pengg <pengg@google.com>
Fuzzing magnificently managed to construct a valid settings byte frame that triggers an assert in test utility code. The assert
ASSERT(result.second)validates that inserting test settings param into an unordered set is successful. The fuzz test generated a settings frame with duplicate settings param and tripped the ASSERT. I take this as a feat of fuzzing.Proposed fix: Official H/2 spec says that settings params replace existing ones (https://tools.ietf.org/html/rfc7540#section-6.5), so erase duplicated and re-insert to reflect this for testing.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=21318
Testing: corpus entry added
Signed-off-by: Asra Ali asraa@google.com