[ovs-dev] [PATCH v2 2/2] Makes stylistic change suggested by 0-day robot.

[Bhargava Shastry]

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, bshastry at sect.tu-berlin.de wrote:
>> From: Bhargava Shastry <bshastry at 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 <bshastry at 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