Page MenuHomePhabricator

[libFuzzer] Diff 20 - Update tests to use more general functions instead of posix specific.

Authored by mpividori on Dec 13 2016, 1:59 PM.



I update the tests to use SearchMemory(), StrStr(), StrCaseStr(), and SleepSeconds(), instead of posix specific functions.

Diff Detail

Event Timeline

mpividori updated this revision to Diff 81295.Dec 13 2016, 1:59 PM
mpividori retitled this revision from to [libFuzzer] Diff 20 - Update tests to use more general functions instead of posix specific..
mpividori updated this object.
mpividori added reviewers: zturner, kcc.
mpividori set the repository for this revision to rL LLVM.
mpividori added a subscriber: llvm-commits.
zturner added inline comments.Dec 13 2016, 2:22 PM
181–182 ↗(On Diff #81295)

Windows has strstr, it just doesn't have strcasestr. I would implement this one using just strstr.

mpividori added inline comments.Dec 13 2016, 2:40 PM
181–182 ↗(On Diff #81295)

@zturner , You are right. I confused because I found the function StrStr() from Shlwapi.lib . I didn't realized that strstr() is part of the std library.
I think it is better to continue with actual implementation of StrCaseStr(). By using std::search(), we don't have no copy the arrays.
If we want to use strstr(), we should copy both arrays, then apply tolower(), and then use strstr(). This can be slower with large arrays.

mpividori updated this revision to Diff 81313.Dec 13 2016, 2:57 PM

Update diff to remove StrStr().

mpividori updated this revision to Diff 81315.Dec 13 2016, 3:10 PM
zturner accepted this revision.Dec 13 2016, 3:19 PM
zturner edited edge metadata.

Agree, we do need StrCaseStr. Looks good to me.

This revision is now accepted and ready to land.Dec 13 2016, 3:19 PM

@kcc would you agree with these changes?

kcc added inline comments.Dec 14 2016, 12:47 PM
5 ↗(On Diff #81665)

Please don't.
I want the tests to remain free of any such deps.

5 ↗(On Diff #81665)


mpividori added inline comments.Dec 14 2016, 1:05 PM
5 ↗(On Diff #81665)

Free from dependencies from libFuzzer?? Even if this tests are linked with the LLVMFuzzer.lib library? You want me to remove the include and declare the function signature? I don't understand...

mpividori added inline comments.Dec 14 2016, 1:09 PM
5 ↗(On Diff #81665)

In CustomCrossOverTest.cpp, CustomMutatorTest.cpp, and other tests, you also include:
#include "FuzzerInterface.h" ...

kcc added inline comments.Dec 14 2016, 1:16 PM
5 ↗(On Diff #81665)

I don't want these tests to depend on libFuzzer in any way.
obviously, declaring the function signature here is even worse.
These tests should be usable with other fuzzing engines.

CustomCrossOverTest.cpp, CustomMutatorTest.cpp,

These are the exceptions because they use cutom mutator/crossover -- something that other fuzzing engines don't support.

What about copying these functions into a common header file that has no dependencies and can be symlinked from the test dir into the source tree? That way the test (which now consists of a cpp file and a h file) is self contained, but there is still only one master copy of the h file so we don't have to worry about code duplication. Then FuzzerUtilWindows.h could just incldue that header file.

(Of course the alternative is just rewrite the function here, which is not the end of the world, although kind of unfortunate)

kcc edited edge metadata.Dec 14 2016, 1:54 PM

(summary of offline discussion).

For OutOfMemoryTest.cpp just use c++11 sleep

For StrstrTest.cpp: this test tests the ability of *a* fuzzing engine to look through strstr and friends.
If some platform does not have e.g. strcasestr, there is no point in implementing it for this test.
a simple

#  define strcasestr strstr

will be good enough

mpividori updated this revision to Diff 81654.Dec 15 2016, 2:18 PM
mpividori edited edge metadata.

@kcc Ok. Let me know if you agree with this final diff.

kcc accepted this revision.Dec 15 2016, 2:21 PM
kcc edited edge metadata.

LGTM with a nit

11 ↗(On Diff #81665)

add a comment:
win32 does not have strcasestr and memmem, so we are not testing them

mpividori updated this revision to Diff 81665.Dec 15 2016, 2:51 PM
mpividori edited edge metadata.


This revision was automatically updated to reflect the committed changes.