Replaced char array in output_stream to std::string to avoid buffer overflow #54
Conversation
mikelik
changed the title
mikelik
force-pushed
the
output_stream_overflow
branch
from
e62ade2
to
5ed84b1
Compare
| CHECK_EQUAL(std_err.index(), large_msg.size()); | ||
|
|
||
| std_err.clear(); | ||
| EOSIO_TEST_END |
There was a problem hiding this comment.
please add get() and push() tests. I see clear(), to_string() and 'index()' calls only
|
|
||
| EOSIO_TEST_BEGIN(output_stream_push_overflow) | ||
| std_err.clear(); | ||
| const auto initial_capacity = std_err.to_string().capacity(); |
There was a problem hiding this comment.
I think it worth to check initial_capaticy as well. if it will be zero test should pass anyway.
There was a problem hiding this comment.
initial capacity of string is unspecified so it can be anything. Do you want to add:
| const auto initial_capacity = std_err.to_string().capacity(); | |
| const auto initial_capacity = std_err.to_string().capacity(); | |
| CHECK_EQUAL(initial_capacity >= 0, true); |
But this will be always true, because it is unsigned_int
There was a problem hiding this comment.
Initially I thought we can check here hardcoded 4kb size but that might be too much of details.
So this looks resolved now.
… and std_err) is capped at 2048. Trying to add more data to output_stream will be silently ignored. Add unit tests for overflow.
Added unit test for get and push.
mikelik
force-pushed
the
output_stream_overflow
branch
from
5ed84b1
to
bb3f985
Compare
|
|
||
| public: | ||
| output_stream() { output.reserve(initial_size); } | ||
| std::string to_string() const { return output; } |
There was a problem hiding this comment.
Was copy an intention here?
const reference might be an alternative
There was a problem hiding this comment.
Right, after we talked yesterday I have changed it, so now it is const ref.
I believe originally it was a copy, because the underlying was a char array.
|
|
||
| EOSIO_TEST_BEGIN(output_stream_push_overflow) | ||
| std_err.clear(); | ||
| const auto initial_capacity = std_err.to_string().capacity(); |
There was a problem hiding this comment.
Initially I thought we can check here hardcoded 4kb size but that might be too much of details.
So this looks resolved now.
Change Description
Solving #12
Replaced char buffer with std::string. Initial capacity of string is set to 4096.
Using output_stream::push should no longer cause buffer overflow.
Added unit tests for overflow.
API Changes
Documentation Additions