This is an archive of the discontinued LLVM Phabricator instance.

[libc] Adds fuzz test for strstr and alphabetizes string fuzz CMakeList.
ClosedPublic

Authored by cgyurgyik on Jul 26 2020, 5:13 PM.

Diff Detail

Event Timeline

cgyurgyik created this revision.Jul 26 2020, 5:13 PM
sivachandra added inline comments.Jul 27 2020, 11:44 PM
libc/fuzzing/string/strstr_fuzz.cpp
23

I am not sure if we should use another implementation of strstr to test the real implementation. Instead, can we use strcmp in a brute force manner as strcmp has its own fuzz test.

auto result = __llvm_libc::strstr(haystack, needle);
// Because of the construction below, the input for which result is NULL can be tested separately.
// For the rest:
if (strcmp(result, needle) != 0) {
  __builtin_trap():
}
auto *haystack_ptr = haystack;
for (;haystack_ptr != result; ++haystack)
    if (strcmp(haystack_ptr, needle) == 0) {
      // An earlier occurance of needle was missed by strstr.
      __builtin_trap();
    }
}

This is brute force and calls strcmp for every byte, but avoids reimplementing strstr. WDYT?

cgyurgyik updated this revision to Diff 281340.Jul 28 2020, 1:12 PM
cgyurgyik updated this revision to Diff 281355.Jul 28 2020, 2:03 PM
cgyurgyik marked an inline comment as not done.
cgyurgyik added inline comments.
libc/fuzzing/string/strstr_fuzz.cpp
23

PTAL and let me know if this is what you're looking for.

I had to make some slight modifications since the result returned will include the rest of the string, and thus strcmp() will not always work.

cgyurgyik added inline comments.Jul 28 2020, 2:13 PM
libc/fuzzing/string/strstr_fuzz.cpp
23

Hmm nevermind I need to make some changes to this.

cgyurgyik updated this revision to Diff 281391.Jul 28 2020, 3:58 PM
cgyurgyik updated this revision to Diff 281396.Jul 28 2020, 4:06 PM
cgyurgyik added inline comments.
libc/fuzzing/string/strstr_fuzz.cpp
23

As discussed, strcmp won't quite work here. Created a helper function that mimics memcmp by comparing each letter up to N.

cgyurgyik marked an inline comment as done.Jul 28 2020, 4:07 PM

Few nits and questions inline but overall LGTM.

libc/fuzzing/string/strstr_fuzz.cpp
22

LLVM style is to use static functions over anonymous namespaces: https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

22

May be call it simple_memcmp.

65

data1 should be copied so that we can add the null terminator. But, why should we copy into data2? Can we not use the input directly?

cgyurgyik marked 3 inline comments as done.
cgyurgyik added inline comments.
libc/fuzzing/string/strstr_fuzz.cpp
65

We can, I mostly did it to show there is no intersection between the two. Fixed so that only one new container is created.

sivachandra accepted this revision.Jul 29 2020, 1:14 PM
This revision is now accepted and ready to land.Jul 29 2020, 1:14 PM
This revision was landed with ongoing or failed builds.Jul 29 2020, 1:36 PM
This revision was automatically updated to reflect the committed changes.
cgyurgyik marked an inline comment as done.