Sorry, I overlooked a third question (addendum to the previous two): 3. Does it make sense to impose a time out for expr_parse_target? The default time out used by oss-fuzz fuzzing engines is 25 seconds i.e., a bug is automatically filed for inputs that take longer than 25 seconds to be processed by the fuzzer target. Is 25 seconds okay? Should it be lower? If yes, I can send another patch that configures the time out in the respective fuzzer.options file.
Thanks, Bhargava On 09/28/2018 01:39 PM, Bhargava Shastry wrote: > 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
