utilities: Implemented an ostream that writes to a user provided buffer by KBaichoo · Pull Request #13797 · envoyproxy/envoy

KBaichoo · 2020-10-28T15:04:25Z

Implemented an ostream that writes to a user provided buffer and truncates if we surpass the buffer size.

Signed-off-by: Kevin Baichoo kbaichoo@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: Implemented an ostream that writes to a user provided buffer and truncates if we surpass the buffer size.
Additional Description: Will be useful for Fatal Actions (see PR #13676 ), and other situations where we want a fixed-sized ostream.
Risk Level: low
Testing: unit tests
Docs Changes: NA
Release Notes: NA
Platform Specific Features: NA

…cates if we surpass the buffer size. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2020-10-28T15:04:55Z

/assign @akonradi

test/common/common/utility_test.cc

akonradi · 2020-10-28T15:18:01Z

test/common/common/utility_test.cc

+  // Can write to buffer
+  {
+    std::string data = "123";
+    std::string buffer(3, 'x');


This and the others could be std::array; that saves you a little on runtime since the contents of the buffer before streaming don't actually matter.

You're right the contents don't matter.

With std::array I'd actually have to loop when calling << which is an antipattern when using ostreams.

What I'll do is just declare a string, and resize() instead.

I don't understand the std::array issue. I was suggesting declaring std::array<uint8, 3> buffer and using std::array::data() and size() below to access the raw bytes.

Now use std::array<char, N> for the buffer, thanks for clarifying

akonradi · 2020-10-28T15:19:07Z

source/common/common/utility.h

+/**
+ * std::ostream class that serializes writes into the provided buffer.
+ */
+class OutputBufferStream : public virtual FixedSizeStreamBuffer, public std::ostream {


This class should probably have a way to provide the number of bytes that were written into the buffer.

test/common/common/utility_test.cc

source/common/common/utility.h

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

source/common/common/utility.h

akonradi · 2020-10-28T18:04:10Z

test/common/common/utility_test.cc

+  // Can write to buffer
+  {
+    std::string data = "123";
+    std::string buffer(3, 'x');


I don't understand the std::array issue. I was suggesting declaring std::array<uint8, 3> buffer and using std::array::data() and size() below to access the raw bytes.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

akonradi

Thanks!

KBaichoo · 2020-10-28T21:42:58Z

PTAL @envoyproxy/senior-maintainers

ggreenway · 2020-10-28T22:46:15Z

We mostly haven't used the iostream model in envoy. What's the motivation/justification for introducing it here?

Or stated a different way: why not have a class with a write() function instead?

KBaichoo · 2020-10-29T12:52:05Z

The motivation for using iostream here is due to the ScopedTrackedObject interface.

One of the use cases for the OutputBufferStream would be as the ostream that the ScopedTrackedObject dumps it state on without allocating additional memory. This is necessary as the ScopedTrackedObject is dumped on a crash and we want to be async-signal-safe which means one thing we shouldn't do is allocate memory.

antoniovicente · 2020-10-29T17:24:05Z

The motivation for using iostream here is due to the ScopedTrackedObject interface.

One of the use cases for the OutputBufferStream would be as the ostream that the ScopedTrackedObject dumps it state on without allocating additional memory. This is necessary as the ScopedTrackedObject is dumped on a crash and we want to be async-signal-safe which means one thing we shouldn't do is allocate memory.

We need a way to capture the output generated by dumpState. The other alternative is to replace use of ostream by dumpState with something else.

KBaichoo · 2020-10-29T17:30:09Z

The other alternative is to replace use of ostream by dumpState with something else.

The benefit of using ostream is that objects implement it in order to be serializable, and the serialization is decoupled from the storage medium (it could be a fixed sized buffer as in this case, it could be a file, etc.)

ggreenway · 2020-11-02T20:40:43Z

The other alternative is to replace use of ostream by dumpState with something else.

The benefit of using ostream is that objects implement it in order to be serializable, and the serialization is decoupled from the storage medium (it could be a fixed sized buffer as in this case, it could be a file, etc.)

Sounds like a reasonable justification to me.

KBaichoo · 2020-11-02T20:58:47Z

/retest

repokitteh-read-only · 2020-11-02T20:58:51Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13797 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo · 2020-11-18T21:37:25Z

friendly ping @ggreenway since the PR #13676 which this will be useful for has merged

ggreenway · 2020-11-18T22:44:12Z

Sorry, I didn't know you were waiting on me for this, and somehow it never got assigned to anyone. I've been pretty busy. @antoniovicente can you take a look?

ggreenway · 2020-11-18T22:45:21Z

Oh, I see what happened: you assigned it to a non-maintainer, so it was never seen in the view for "unassigned PRs" to get assigned to a maintainer.

antoniovicente

Very nice. I expected this to be more complicated to implement. Still some regrets about this not being something provided out of the box by C++ standard libraries, since it does provide all the tools needed to do this.

test/common/common/utility_test.cc

antoniovicente · 2020-11-19T03:25:18Z

test/common/common/utility_test.cc

+
+TEST(OutputBufferStream, CanWriteToBuffer) {
+  constexpr char data[] = "123";
+  std::array<char, 3> buffer;


I forget if ASAN is able to find issues with data members like this allocated in the stack. Consider instead:

std::unique_ptr<char[]> buffer(new char[3]);

ASAN doesn't complain about it.

If you were to do something like:

OutputBufferStream ostream{buffer.data(), buffer.size()+1};
ostream << "lots of bytes";

Would ASAN detect the problem?

Yes ASAN detected that.

We're saying the buffer is bigger than it is (i.e. buffer.size() + 1) and trying to write to memory we don't have in that case.

Great. std::array works for me.

antoniovicente · 2020-11-19T03:35:33Z

source/common/common/utility.cc

+}
+
+OutputBufferStream::OutputBufferStream(char* data, size_t size)
+    : FixedSizeStreamBuffer{data, size}, std::ostream{static_cast<std::streambuf*>(this)} {}


Does the call to setp need to happen before a pointer is given to the ostream constructor?

This seems to work:
/**
* std::ostream class that serializes writes into the provided buffer.
*/
class OutputBufferStream : private std::streambuf, public std::ostream {
public:
OutputBufferStream(char* data, size_t size);
...
}

OutputBufferStream::OutputBufferStream(char* data, size_t size)
: std::ostream(this) {
setp(data, data + size);
}

The call to setp doesn't need to occur first since streambuf are more focused on managing some storage (buffer, file, ...) and ostreams are more focused on serializing objects. ostream defers the actual writing / storage management to the streambuf.

I organized it like this to be consistent with the InputConstMemoryStream. If you want I can flatten it (removing FixedSizeStreamBuffer)

I would consider using the name MutableMemoryStreamBuffer

I don't feel strongly about flattening it. Flattening it seems slightly cleaner but both approaches are fine.

source/common/common/utility.h

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo

Thanks for the review!

source/common/common/utility.h

KBaichoo · 2020-11-19T15:56:18Z

test/common/common/utility_test.cc

+
+TEST(OutputBufferStream, CanWriteToBuffer) {
+  constexpr char data[] = "123";
+  std::array<char, 3> buffer;


ASAN doesn't complain about it.

test/common/common/utility_test.cc

KBaichoo · 2020-11-19T16:18:05Z

source/common/common/utility.cc

+}
+
+OutputBufferStream::OutputBufferStream(char* data, size_t size)
+    : FixedSizeStreamBuffer{data, size}, std::ostream{static_cast<std::streambuf*>(this)} {}


The call to setp doesn't need to occur first since streambuf are more focused on managing some storage (buffer, file, ...) and ostreams are more focused on serializing objects. ostream defers the actual writing / storage management to the streambuf.

I organized it like this to be consistent with the InputConstMemoryStream. If you want I can flatten it (removing FixedSizeStreamBuffer)

antoniovicente · 2020-11-20T19:38:27Z

source/common/common/utility.cc

+}
+
+OutputBufferStream::OutputBufferStream(char* data, size_t size)
+    : FixedSizeStreamBuffer{data, size}, std::ostream{static_cast<std::streambuf*>(this)} {}


I would consider using the name MutableMemoryStreamBuffer

I don't feel strongly about flattening it. Flattening it seems slightly cleaner but both approaches are fine.

antoniovicente · 2020-11-20T19:40:19Z

test/common/common/utility_test.cc

+
+TEST(OutputBufferStream, CanWriteToBuffer) {
+  constexpr char data[] = "123";
+  std::array<char, 3> buffer;


If you were to do something like:

OutputBufferStream ostream{buffer.data(), buffer.size()+1};
ostream << "lots of bytes";

Would ASAN detect the problem?

antoniovicente · 2020-11-20T19:42:27Z

test/common/common/utility_test.cc

+  EXPECT_EQ(ostream.contents(), "12");
+}
+
+TEST(OutputBufferStream, ReturnsNumberOfBytesWrittenIntoBuffer) {


This test seems very similar to OutputBufferStream.CanWriteToBuffer

Folded into that test and the overflow test.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo

Thanks for the review.

KBaichoo · 2020-11-20T19:56:19Z

test/common/common/utility_test.cc

+  EXPECT_EQ(ostream.contents(), "12");
+}
+
+TEST(OutputBufferStream, ReturnsNumberOfBytesWrittenIntoBuffer) {


Folded into that test and the overflow test.

KBaichoo · 2020-11-20T20:03:04Z

source/common/common/utility.cc

+}
+
+OutputBufferStream::OutputBufferStream(char* data, size_t size)
+    : FixedSizeStreamBuffer{data, size}, std::ostream{static_cast<std::streambuf*>(this)} {}


KBaichoo · 2020-11-20T20:12:12Z

test/common/common/utility_test.cc

+
+TEST(OutputBufferStream, CanWriteToBuffer) {
+  constexpr char data[] = "123";
+  std::array<char, 3> buffer;


Yes ASAN detected that.

We're saying the buffer is bigger than it is (i.e. buffer.size() + 1) and trying to write to memory we don't have in that case.

antoniovicente · 2020-11-20T22:20:08Z

test/common/common/utility_test.cc

+
+TEST(OutputBufferStream, CanWriteToBuffer) {
+  constexpr char data[] = "123";
+  std::array<char, 3> buffer;


Great. std::array works for me.

antoniovicente · 2020-11-20T23:54:54Z

Could you merge master to see if that fixes the coverage error?

2020-11-20T22:29:10.8563882Z Per-extension coverage failed:
2020-11-20T22:29:10.8565398Z Code coverage for source/common/event is lower than limit of 93.5 (92.9)

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente · 2020-11-23T20:52:56Z

cc @asraa

Thoughts on what may be up with linux_x64 fuzz_coverage? Run seems to still be pending after more than 5 hours.

asraa · 2020-11-23T20:55:25Z

I'm going to guess it's an AZP issue. The build aborted midway
##[error]We stopped hearing from agent i-0c54ce55437fb840e.
time="2020-11-23T15:01:24Z" level=error msg="error waiting for container: unexpected EOF"

I'll re-run and keep the tab open.

…er (envoyproxy#13797) Signed-off-by: Kevin Baichoo <kbaichoo@google.com> Signed-off-by: Qin Qin <qqin@google.com>

Implemented an ostream that writes to a user provided buffer and trun…

a6af696

…cates if we surpass the buffer size. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot assigned akonradi Oct 28, 2020

akonradi suggested changes Oct 28, 2020

View reviewed changes

Minor changes to tests mostly.

ac8a034

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

akonradi reviewed Oct 28, 2020

View reviewed changes

Marked numValidBytesWrittenToBuffer const. Used std::array for test.

6f23391

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

akonradi previously approved these changes Oct 28, 2020

View reviewed changes

ggreenway assigned antoniovicente Nov 18, 2020

antoniovicente reviewed Nov 19, 2020

View reviewed changes

KBaichoo added 2 commits November 19, 2020 14:56

Merge remote-tracking branch 'upstream/master' into ostream-memory-buff

e67164e

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Added contents() method, cleaned up naming, etc.

aada91b

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo dismissed akonradi’s stale review via aada91b November 19, 2020 16:21

KBaichoo commented Nov 19, 2020

View reviewed changes

antoniovicente reviewed Nov 20, 2020

View reviewed changes

Minor tweaks.

9c75aac

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo commented Nov 20, 2020

View reviewed changes

antoniovicente approved these changes Nov 20, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into ostream-memory-buff

24513e3

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

mattklein123 merged commit c41850c into envoyproxy:master Nov 23, 2020

qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020

utilities: Implemented an ostream that writes to a user provided buff…

c0f5cff

…er (envoyproxy#13797) Signed-off-by: Kevin Baichoo <kbaichoo@google.com> Signed-off-by: Qin Qin <qqin@google.com>

Conversation

KBaichoo commented Oct 28, 2020

Uh oh!

KBaichoo commented Oct 28, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akonradi left a comment

Choose a reason for hiding this comment

Uh oh!

KBaichoo commented Oct 28, 2020

Uh oh!

ggreenway commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KBaichoo commented Oct 29, 2020

Uh oh!

antoniovicente commented Oct 29, 2020

Uh oh!

KBaichoo commented Oct 29, 2020

Uh oh!

ggreenway commented Nov 2, 2020

Uh oh!

KBaichoo commented Nov 2, 2020

Uh oh!

repokitteh-read-only bot commented Nov 2, 2020

Uh oh!

KBaichoo commented Nov 18, 2020

Uh oh!

ggreenway commented Nov 18, 2020

Uh oh!

ggreenway commented Nov 18, 2020

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Oct 28, 2020 •

edited

Loading