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, [email protected] wrote: >> From: Bhargava Shastry <[email protected]> >> >> 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 <[email protected]> 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
