Core: create buffer utility class + buffer unit-test + RI buffer refactor #926
Conversation
| reinterpret_cast<std::uint8_t*>(m_Buffer.get()) + m_BufferLen); | ||
| m_Buffer.data(), | ||
| m_Buffer.size(), | ||
| m_Buffer.data() + m_Buffer.size()); |
There was a problem hiding this comment.
These changes are definitely better that what's currently in place.
This is potentially dangerous if router_info.Str().size() + private_keys.GetPublic().GetSignatureLen() is greater than Size::MaxBuffer, since it would write past the end of the buffer, without throwing an error. With the changes in #917, this will be a non-issue here, but may come up in other places.
See my comment about a suggested test for proof of a SIGABRT.
There was a problem hiding this comment.
This bug pre-existed the buffer util changes, and buffer util makes the situation better. With the changes in the signature PR, on top of this PR, this bug will no longer exist.
There was a problem hiding this comment.
Yes, this is a dangerous and stupid way to do things, but this applies to any underlying pointer handling (and this is legacy code btw). I imagined you'd rebase #917 and that's good because RI needs further refactoring.
There was a problem hiding this comment.
I imagined you'd rebase #917 and that's good because RI needs further refactoring.
This is exactly what I intend to do. Happy to be helping fix the legacy code.
tests/unit_tests/core/util/buffer.cc
Outdated
|
|
||
| buf(data.data(), data.size()); | ||
| BOOST_CHECK(buf.get() == data); | ||
| } |
There was a problem hiding this comment.
I'm not sure how to catch the stack smashing error with Boost.Test, but wanted to simulate the exceptional case of an overwrite during RouterInfo signing.
Here is the test case that produces the error:
diff --git a/tests/unit_tests/core/util/buffer.cc b/tests/unit_tests/core/util/buffer.cc
index fbed86ef..88fa4721 100644
--- a/tests/unit_tests/core/util/buffer.cc
+++ b/tests/unit_tests/core/util/buffer.cc
@@ -97,6 +97,15 @@ BOOST_AUTO_TEST_CASE(Data)
BOOST_CHECK(buf.get() == data);
}
+BOOST_AUTO_TEST_CASE(DataOverwrite)
+{
+ std::array<std::uint8_t, Max> data{{}};
+ std::array<std::uint8_t, 64> extra{{}};
+
+ BOOST_CHECK_NO_THROW(buf(data.data(), data.size()));
+ memcpy(buf.data() + buf.size(), extra.data(), extra.size());
+}
+the output from the suite:
Running 166 test cases...
*** stack smashing detected ***: <unknown> terminated
unknown location(0): fatal error: in "Buffer/DataOverwrite": signal: SIGABRT (application abort requested)
/kovri/tests/unit_tests/core/util/buffer.cc(100): last checkpoint: "DataOverwrite" fixture dtor
*** 1 failure is detected in the test module "Master Test Suite"
The same error occurs using PrivateKeys::Sign like in the RouterInfo impl.
There was a problem hiding this comment.
I can't reproduce. What compiler are you using?
There was a problem hiding this comment.
Note: none of the buildbot compilers (the ones that are online) can reproduce the issue. I can't reproduce with clang6. After testing with gcc8 though, I can reproduce.
buf.data() + buf.size()
But what do you expect, you're telling the compiler to do a raw copy beyond the bounds of the buffer... You will get the same thing with this:
std::array<std::uint8_t, Max> arr{{}};
std::array<std::uint8_t, Max> data{{}};
std::memcpy(arr.data() + arr.size(), data.data(), data.size());Are you sure you weren't trying to do this?:
diff --git a/tests/unit_tests/core/util/buffer.cc b/tests/unit_tests/core/util/buffer.cc
index fbed86ef..911948f1 100644
--- a/tests/unit_tests/core/util/buffer.cc
+++ b/tests/unit_tests/core/util/buffer.cc
@@ -121,4 +121,11 @@ BOOST_AUTO_TEST_CASE(InvalidBuffer)
BOOST_CHECK_THROW(core::Buffer<> buf(Max + 1), std::exception);
}
+BOOST_AUTO_TEST_CASE(DataOverwrite)
+{
+ core::Buffer<0, 32> bad;
+ std::array<std::uint8_t, Max> data{{}};
+ BOOST_CHECK_THROW(bad(data.data(), data.size()), std::exception);
+}
+
BOOST_AUTO_TEST_SUITE_END()There was a problem hiding this comment.
That case shows the expected failure much better. The one I posted was to show the danger of the previous impl, and why resolving the signature TODO is important.
Resolving the signature TODO will be much safer and straightforward with this buffer util.
There was a problem hiding this comment.
I've added my test case (thanks for pointing that out, I thought I had checked the functor in one of the previous cases).
The one I posted was to show the danger of the previous impl
Raw pointers are gonna raw point. Let's get rid of them when possible. This buffer util unfortunately only bandaids the problem because we really need to use pure containers whenever possible (even when interfacing with legacy code).
| namespace core | ||
| { | ||
| // TODO(anonimal): Boost.Buffer? | ||
| // TODO(anonimal): SecBlock buffer |
There was a problem hiding this comment.
FWIW, my vote is for SecBlock, then we could also use this class for PrivateKeys.
There was a problem hiding this comment.
Yes. The SecBlock family was a definitive yes (#784). As for the boost.buffer question, that's for this underlying non-SecBlock implementation.
Was not rebased after the buffer refactor in monero-project#926
By submitting this pull-request, I confirm the following:
This was getting in the way of me finishing my
bandcapsbranch. Otherwise, I'd have left the TODO.RouterInfo is still very messy. Pulling it apart and a writing unit-test will be beneficial. Referencing #627.