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

[Bhargava Shastry]

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, 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