util: Add MemBlockBuilder class to help manage blocks of memory for safe memcpy by jmarantz · Pull Request #9306 · envoyproxy/envoy

jmarantz · 2019-12-11T02:31:32Z

Description: Split out from #8779 per suggestion from @htuch -- adds a class to help manage memory blocks, to make it easier to reason about the correctness of calls to memcpy.
Risk Level: low -- this PR just adds/tests a new utility class
Testing: just the new test
Docs Changes: n/a
Release Notes: n/a

…mcpy. Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch

LGTM modulo comments, thanks, this is a great improvement IMHO.
/wait

source/common/common/mem_block_builder.h

test/common/common/mem_block_builder_test.cc

source/common/common/mem_block_builder.h

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch

Awesome, this is a really nice contribution, @envoyproxy/maintainers please encourage the use of this pattern going forward where applicable.

htuch · 2019-12-12T21:19:57Z

@jmarantz looks like legit CI failures on coverage related to this PR, want to take a look?

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-12-13T20:08:10Z

woo-hoo: this squirrels by the coverage issues matching the expected error message; ptal.

jmarantz · 2019-12-13T20:32:34Z

@envoyproxy/maintainers can I have a non-googler review? This is should be relatively simple, as this adds a new class & tests but does not use it yet.

mattklein123

Nice. A few comments for discussion.

/wait-any

source/common/common/mem_block_builder.h

mattklein123 · 2019-12-13T23:04:22Z

source/common/common/mem_block_builder.h

+   * @param data The block of objects to insert.
+   * @param size The number of elements in the block.
+   */
+  void appendData(const T* data, uint64_t size) {


How about using absl::span here? Seems more flexible and a bit more type checked for many uses?

I'm glad you mentioned this because I did not know about absl::Span<>. Pretty cool; I'll keep that one in mind. But I don't see how that data structure helps in this case, as the functionality it automates isn't really what this class does, which is provide a builder-like interface for filling in an owned piece of data.

You are passing const T* data, uint64_t size which can be expressed in a span, just like a string_view. It seemed a more flexible interface that does the same thing? nbd either way.

I see...ok that definitely improves the interface. Thanks!

I also did -- in a separate commit -- a change to use a span in the rep as well. That reduces the number of lines of code in this impl. Note than in absl::Span::remove_prefix() there is an assert() already, so I removed the SECURITY_ASSERT for now, and key off that in the death-tests.

I feel like the change in AppendOne from *write_ptr_++ = ch to write_span_[0] = ch; write_span_.remove_prefix(1); is a little less intuitive and might not compile to something quite as fast. WDYT? I can always roll back e0d6ee1

source/common/common/mem_block_builder.h

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…at check the redundancy. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

… correct assertion. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…e disabled for optimized builds. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

Nice, thanks.

mattklein123 · 2019-12-15T16:46:58Z

source/common/common/mem_block_builder.h

+   *
+   * @param capacity The number of memory elements to allocate.
+   */
+  void populate(uint64_t capacity) { populateHelper(capacity, std::make_unique<T[]>(capacity)); }


nit: I would probably call this resize(...) but up to you.

Yeah that's better; will adjust in a follow-up though.

mattklein123 · 2019-12-15T16:50:52Z

source/common/common/mem_block_builder.h

+    *write_span_.data() = object;
+    write_span_.remove_prefix(1);


Agreed this is not quite as intuitive but I'm fine either way.

i'll see how this feels for now. Thanks.

…afe memcpy (envoyproxy#9306) * Add MemBlockBuilder class to help manage blocks of memory for safe memcpy. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

Add MemBlockBuilder class to help manage blocks of memory for safe me…

13c243e

…mcpy. Signed-off-by: Joshua Marantz <jmarantz@google.com>

dio assigned htuch Dec 11, 2019

jmarantz mentioned this pull request Dec 11, 2019

stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy #8779

Merged

htuch suggested changes Dec 11, 2019

View reviewed changes

source/common/common/mem_block_builder.h Outdated Show resolved Hide resolved

test/common/common/mem_block_builder_test.cc Show resolved Hide resolved

source/common/common/mem_block_builder.h Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Dec 11, 2019

jmarantz mentioned this pull request Dec 12, 2019

util: memcpy of whole structures should have a macro wrapper #9328

Closed

fix typo, add death tests.

4665e8e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label Dec 12, 2019

jmarantz added 2 commits December 12, 2019 08:27

Merge branch 'master' into memblock

092cd92

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Use a regex that matches as a substring.

3ba67c8

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch previously approved these changes Dec 12, 2019

View reviewed changes

Try putting the death tests in two different test-methods.

181148a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed htuch’s stale review via 181148a December 12, 2019 21:30

jmarantz added 3 commits December 13, 2019 11:47

Don't look for the particular error message when building for coverage.

bf7c029

Signed-off-by: Joshua Marantz <jmarantz@google.com>

format

63f9a72

Signed-off-by: Joshua Marantz <jmarantz@google.com>

try more coverage-mode workarounds.

49502dc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch previously approved these changes Dec 13, 2019

View reviewed changes

jmarantz assigned mattklein123 Dec 13, 2019

mattklein123 reviewed Dec 13, 2019

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Dec 13, 2019

jmarantz added 2 commits December 13, 2019 21:23

review comments (pre-span)

9261d28

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove redundant member variable capacity_, as well as the asserts th…

aa96da5

…at check the redundancy. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed htuch’s stale review via aa96da5 December 14, 2019 02:34

jmarantz added 3 commits December 14, 2019 08:07

Use spans at the interface rather than ptr,size

87bd604

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Use spans for the representation too.

e0d6ee1

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Run remove_prefix prior to writing data, so that death-tests show the…

4f1030a

… correct assertion. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 2 commits December 14, 2019 10:28

Put back our own SECURITY_ASSERT as the assert() in absl appears to b…

48679fd

…e disabled for optimized builds. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Update doxygen for appendData().

e915ea3

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 approved these changes Dec 15, 2019

View reviewed changes

jmarantz merged commit 7f4c687 into envoyproxy:master Dec 15, 2019

jmarantz deleted the memblock branch December 15, 2019 17:39

jmarantz mentioned this pull request Dec 16, 2019

util: Add test for MemBlockBuilder::release(), which was not working (discovered in #8779). #9360

Merged

Conversation

jmarantz commented Dec 11, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Dec 12, 2019

Uh oh!

jmarantz commented Dec 13, 2019

Uh oh!

jmarantz commented Dec 13, 2019

Uh oh!

mattklein123 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!

Uh oh!

mattklein123 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants