fix SSL write crash when write buffer has a very large chain#592
fix SSL write crash when write buffer has a very large chain#592mattklein123 merged 2 commits intomasterfrom
Conversation
Previously, we would get all buffer slices on the stack and stack overflow if the chain was very large. This change limits the number of slices that we write during each iteration. There are a number of potential improvements possible in terms of collapsing small moves, enforcing fairness in terms of number of iterations, etc. This commit also removes a bunch of RawSlice to evbuffer_iovec conversions for performance reasons. NOTE: The added test repros the crash before the fix. fixes #585
|
@lyft/network-team @htuch |
source/common/ssl/connection_impl.cc
Outdated
| } | ||
| uint64_t total_bytes_written = 0; | ||
| bool keep_writing = true; | ||
| while (write_buffer_.length() && keep_writing) { |
There was a problem hiding this comment.
Nit: Prefer explicit write_buffer_.length() > 0.
| } | ||
|
|
||
| return {PostIoAction::KeepOpen, bytes_written}; | ||
| return {PostIoAction::KeepOpen, total_bytes_written}; |
There was a problem hiding this comment.
There are other locations where the Buffer::RawSlices slices[num_slices] pattern appears (from a grep), e.g. source/common/http/http1/codec_impl.cc. Naively it seems similar concerns would exist there. Is there an easy way to avoid this stack overflow danger globally?
There was a problem hiding this comment.
Unfortunately there is no global way to do this, since the code currently assumes that it returns all slices, or the size of the slices array required to hold them all. I will put in a TODO to figure this out and clean this up globally. I want to get a fix for this crash out since we saw this in production.
| if (bytes_written > 0) { | ||
| write_buffer_.drain(bytes_written); | ||
| if (inner_bytes_written > 0) { | ||
| write_buffer_.drain(inner_bytes_written); |
There was a problem hiding this comment.
Is there any advantage to draining outside the loop instead of on each iteration?
There was a problem hiding this comment.
If we don't drain during the loop, we keep getting the same slices at the beginning. I will put in a comment.
| // RawSlice is the same structure as evbuffer_iovec. This was put into place to avoid leaking | ||
| // libevent into most code since we will likely replace evbuffer with our own implementation at | ||
| // some point. However, we can avoid a bunch of copies since the structure is the same. | ||
| static_assert(sizeof(RawSlice) == sizeof(evbuffer_iovec), "RawSlice != evbuffer_iovec"); |
|
@htuch updated |
|
LGTM. |
This is a regression from #592 due to some truly awesome libevent behavior. See commit comments for more info.
This is a regression from #592 due to some truly awesome libevent behavior. See commit comments for more info.
Automatic merge from submit-queue [DO NOT MERGE] Auto PR to update dependencies of proxy This PR will be merged automatically once checks are successful. ```release-note none ```
Signed-off-by: John Plevyak <jplevyak@gmail.com>
…nvoyproxy#616) Signed-off-by: John Plevyak <jplevyak@gmail.com>
…se-116 [release-1.16] 2023 04 04 release 116
**Commit Message** This was too strict as a repo-wide rule, so this commit removes it. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
3 participants
Previously, we would get all buffer slices on the stack and stack overflow
if the chain was very large. This change limits the number of slices that we
write during each iteration. There are a number of potential improvements
possible in terms of collapsing small moves, enforcing fairness in terms of
number of iterations, etc.
This commit also removes a bunch of RawSlice to evbuffer_iovec conversions
for performance reasons.
NOTE: The added test repros the crash before the fix.
fixes #585