Hi Ben, Please ignore my comment regarding build (under the heading === 1. Build ===). There was a bug in my earlier patch which I fixed in my latest patch. Please don't rename anything in OvS repo or in the ovs-fuzzing-corpus repo. Local tests show everything works as expected.
However, two questions remain: 1. Does it make sense to move the fuzzer build script to Openvswitch repo in the long run? 2. Does it make sense to impose a size limit on inputs to the expr_parse_fuzzer target. Thanks, Bhargava On 09/28/2018 12:42 PM, Bhargava Shastry wrote: > Hi Ben, > > Thanks for taking a close look applying the (edited) patch. I have a > couple of comments: > > === 1. Build === > > Currently, all fuzzer test harnesses in OvS are built by this bash > script located at Google oss-fuzz repo [1]. The peculiar thing about > Google's fuzz infrastructure is that it expects fuzzer configuration and > seed corpus files to share the name of the fuzzer target. > > [1]: > https://github.com/google/oss-fuzz/blob/master/projects/openvswitch/build.sh > > For instance, let's assume that the OvS build system produces a linked > fuzzer target called "openvswitch_expr_parse_target." Google's fuzzing > infra expects the configuration for this target to be called > "openvswitch_expr_parse_target.options" i.e., > <fuzzer_target_name>.options and the seed corpus to be called > "openvswitch_expr_parse_target_seed_corpus.zip" i.e., > <fuzzer_target_name>_seed_corpus.zip. > > Now, the build script in the Google oss-fuzz repo (see [1]) takes care > of constructing the seed_corpus zip file. However, it expects the corpus > folder name to match the fuzzer target. Likewise for the fuzzer > configuration file. > > The problem with the current patch integration is that the OvS build, > for some reason that I cannot comprehend (my make knowledge is poor), > creates a fuzzer target called "openvswitch_expr_parse_target" i.e., it > prefixes the string "openvswitch_" to its namesake C file, > expr_parse_target.c. One outcome of this (unexpected) change in naming > convention is that the fuzzer configuration options and seed corpora are > not picked up by oss-fuzz. For example, one of the fuzzer config options > asks the fuzzer to **not** generate inputs greater than 100 characters > but as you can see by a recent bug report, the input size to trigger the > bug is 8 kB. This brings me to my second concern. But before that, I > have a suggestion to address this issue: > > Short-term: In the short term it suffices to rename the config files and > seed corpora according to the linked fuzzer target. This would mean > renaming the folder called "expr_parse_seed_corpus" to > "openvswitch_expr_parse_seed_corpus" in the openvswitch > ovs-fuzzing-corpus repo [2] AND renaming the fuzzer options file > (located at 'tests/oss-fuzz/config') "expr_parse_target.options" to > "openvswitch_expr_parse_target.options" > > Long-term: In the long term, it may be wise to move the fuzzer build > script from the Google oss-fuzz repo to the Open vSwitch repo and > maintain it alongside OvS code. The Google oss-fuzz repo can then > contain a bash one liner like so: > > ``` > ./tests/oss-fuzz/build.sh > ``` > > that invokes OvS fuzzer build script. Perhaps, a README to this effect > can be added to aid maintenance. > > === 2. Input size === > > I am not sure I completely understand the input specification of the OVN > parser. Should we impose a limit on the number of characters parsed by > the lexer/expression parser? The decision to make it 100 characters in > my earlier patch is ad-hoc. My reasoning was that these strings are > sourced from some sort of human (network administrator) input and hence > they are typically short. However, should it be sourced from a config > file of some sort, perhaps not imposing a character limit is good to > help the fuzzer tease out all sorts of corner cases. > > So, here's my question. Currently, you can see that the fuzzer options > file for the ovn target > (tests/oss-fuzz/config/expr_parse_target.options) has a line to this effect: > > max_len = 100 > > Do you suggest keeping it this way, doing away with it, or increasing it > to a higher threshold? > > Thanks, > Bhargava > > On 09/27/2018 11:27 PM, Ben Pfaff wrote: >> On Thu, Sep 27, 2018 at 02:07:42PM +0200, bshas...@sect.tu-berlin.de wrote: >>> From: Bhargava Shastry <bshas...@sect.tu-berlin.de> >>> >>> Wraps overly long line in expr_parse_target.c >>> >>> Signed-off-by: Bhargava Shastry <bshastry at sect.tu-berlin.de> >> >> I folded this into the first patch. Thanks! >> > -- Bhargava Shastry <bshas...@sect.tu-berlin.de> Security in Telecommunications TU Berlin / Telekom Innovation Laboratories Ernst-Reuter-Platz 7, Sekr TEL 17 / D - 10587 Berlin, Germany phone: +49 30 8353 58235 Keybase: https://keybase.io/bshastry _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev