New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fuzz: utility fuzz test #3493

Merged
merged 5 commits into from Jun 7, 2018

Conversation

Projects
None yet
2 participants
@anirudhmurali
Copy link
Member

anirudhmurali commented May 28, 2018

Title: fuzz: utility fuzz test

Description: This provides fuzzing for few of the string functions present in source/common/common/utility.cc

Risk Level: Low

Testing: Tested under oss-fuzz and unit tests (bazel test //test/common/common:utility_fuzz_test)

Signed-off-by: Anirudh M m.anirudh18@gmail.com

fuzz: utility fuzz test.
This provides fuzzing for few of the string functions present in `source/common/common/utility.cc`

Risk Level: Low
Testing: Tested under oss-fuzz and unit tests

Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch
Copy link
Member

htuch left a comment

Nice start to the summers work.

DEFINE_FUZZER(const uint8_t* buf, size_t len) {

uint64_t out;
std::string stringBuffer(*buf, len);

This comment has been minimized.

@htuch

htuch May 29, 2018

Member

I think you want this to be (buf, len), rather than (*buf, len). Otherwise, it's just creating a fixed string :)

DEFINE_FUZZER(const uint8_t* buf, size_t len) {

uint64_t out;
std::string stringBuffer(*buf, len);

This comment has been minimized.

@htuch

htuch May 29, 2018

Member

You need to use string_buffer, not stringBuffer for locals in Envoy style, please take a look at the style guide at https://github.com/envoyproxy/envoy/blob/master/STYLE.md if not familiar.


uint64_t out;
std::string stringBuffer(*buf, len);
absl::string_view abslString = stringBuffer;

This comment has been minimized.

@htuch

htuch May 29, 2018

Member

This shouldn't be needed, it will be implicitly converted to a string view as needed AFAICT.

std::string stringBuffer(*buf, len);
absl::string_view abslString = stringBuffer;

if (len > 0) {

This comment has been minimized.

@htuch

htuch May 29, 2018

Member

You should still do this when 0 to test the empty case.

This comment has been minimized.

@anirudhmurali

anirudhmurali Jun 1, 2018

Member

size_t len being an unsigned integer, is always positive. So, no need of this check, removed it.

Envoy::StringUtil::escape(stringBuffer);
Envoy::StringUtil::endsWith(stringBuffer, stringBuffer);
Envoy::StringUtil::caseCompare(abslString, abslString);
Envoy::StringUtil::cropLeft(abslString, abslString);

This comment has been minimized.

@htuch

htuch May 29, 2018

Member

For the multi parameter string tests, I think you want to split the buffer and use the first part in the first arg as a string, and the second part as the second arg. You could take the first few bytes of the vector, use that to determine the split point.

Envoy::StringUtil::toUpper(abslString);
Envoy::StringUtil::trim(abslString);
Envoy::StringUtil::ltrim(abslString);
Envoy::StringUtil::rtrim(abslString);

This comment has been minimized.

@htuch

htuch May 29, 2018

Member

You probably want to make each of these a separate case, where you copy the original string, to avoid having earlier functions affect later ones. E.g.


{
  std::string arg(string_buffer);
  Envoy::StringUtil::ltrim(arg);
}
{
  std::string arg(string_buffer);
  Envoy::StringUtil::rtrim(arg);
}

anirudhmurali added some commits Jun 1, 2018

fuzz: address PR review comments
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
fuzz: fix format
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch
Copy link
Member

htuch left a comment

Great, next round of feedback. We can probably merge once nits are addressed if you want to work on the split point aspect.

*/
{
std::string string_buffer(reinterpret_cast<const char*>(buf), len);
Envoy::StringUtil::atoul(string_buffer.c_str(), out);

This comment has been minimized.

@htuch

htuch Jun 4, 2018

Member

Nit: You can drop the Envoy:: prefix here, since we're in the Envoy namespace in the test.

* end.
*/
{
std::string string_buffer(reinterpret_cast<const char*>(buf), len);

This comment has been minimized.

@htuch

htuch Jun 4, 2018

Member

Nit: a bunch of these std::string can be also const.

DEFINE_FUZZER(const uint8_t* buf, size_t len) {
uint64_t out;
/**
* @param string_buffer.substr(len / 2, len / 2) denotes the part from half of the buffer till the

This comment has been minimized.

@htuch

htuch Jun 4, 2018

Member

Nit: we only use Doxygen style for function parameters, you can just do a regular blocks of // comments here.

namespace Fuzz {

DEFINE_FUZZER(const uint8_t* buf, size_t len) {
uint64_t out;

This comment has been minimized.

@htuch

htuch Jun 4, 2018

Member

Nit: this can go inside the atoul scope where it is used. More precise scoping is always best.

}
{
std::string string_buffer(reinterpret_cast<const char*>(buf), len);
Envoy::StringUtil::endsWith(string_buffer, string_buffer.substr(len / 2, len / 2));

This comment has been minimized.

@htuch

htuch Jun 4, 2018

Member

This is fine as a starting point, but it would be interesting to also see what happens if we say "use the first 4 bytes of the buffer as an offset modulo the length of the buffer for the split point". That can be a followup PR if you'd like to land this one first.

fuzz: address PR review comments
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>

@anirudhmurali anirudhmurali force-pushed the anirudhmurali:fuzz-utility branch from 9a161ed to 8777892 Jun 4, 2018

const std::string string_buffer(reinterpret_cast<const char*>(buf), len);
StringUtil::rtrim(string_buffer);
}
if (len > 4) {

This comment has been minimized.

@htuch

htuch Jun 5, 2018

Member

Thinking about this a bit more, I think using a uint8_t would probably be sufficient for the purposes of fuzzing these utilities. That means you could make this len > 0, which would be sufficient to cover all expected cases. When len == 0, we should already have manual tests (and if not can add them).

StringUtil::rtrim(string_buffer);
}
if (len > 4) {
int split_point = *reinterpret_cast<const uint32_t*>(buf) % len;

This comment has been minimized.

@htuch

htuch Jun 5, 2018

Member

Make this a const size_t

{
const std::string string_buffer(reinterpret_cast<const char*>(buf), len);
StringUtil::endsWith(string_buffer.substr(0, split_point),
string_buffer.substr(split_point + 1, std::string::npos));

This comment has been minimized.

@htuch

htuch Jun 5, 2018

Member

I think you just want this to be split_point rather than split_point + 1, since the previous string extends from [0..split_point - 1]. Also, you can skip the npos arg, it's the default arg value.

fuzz: address PR review comments
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch

htuch approved these changes Jun 7, 2018

Copy link
Member

htuch left a comment

Great, nice first PR.

@htuch htuch merged commit caea847 into envoyproxy:master Jun 7, 2018

12 checks passed

DCO All commits have a DCO sign-off from the author
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: asan Your tests passed on CircleCI!
Details
ci/circleci: build_image Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: filter_example_mirror Your tests passed on CircleCI!
Details
ci/circleci: format Your tests passed on CircleCI!
Details
ci/circleci: ipv6_tests Your tests passed on CircleCI!
Details
ci/circleci: mac Your tests passed on CircleCI!
Details
ci/circleci: release Your tests passed on CircleCI!
Details
ci/circleci: tsan Your tests passed on CircleCI!
Details

@htuch htuch referenced this pull request Jun 7, 2018

Closed

fuzzing support #508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment