Libguestfs - Aug 2019 - Re: [PATCH libnbd 7/9] generator: On entry to API functions, check Flags and OFlags parameters.

On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
> Generate checks that no unknown (at the time of compilation) flags are
> passed to Flags or OFlags parameters.
> ---
>  generator/generator | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
We may still want to introduce a testing mode where you can use libnbd
to drive unknown flags to a server that understands them, for
experimenting with protocol extensions not yet included in the NBD
specification. But if we ever do add such a mode, it shouldn't be hard
to make this sanity checking conditional on that mode.
> 
> diff --git a/generator/generator b/generator/generator
> index 96d1148..a6aea26 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -3689,6 +3689,19 @@ let generate_lib_api_c () >      );
>  
>      (* Check parameters are valid. *)
> +    let print_flags_check n { flag_prefix; flags } > +      let value =
match errcode with
> +        | Some value -> value
> +        | None -> assert false in
> +      let mask = List.fold_left (lor) 0 (List.map snd flags) in
> +      pr "  if ((%s & ~%d) != 0) {\n" n mask;
> +      pr "    set_error (EINVAL, \"%%s: invalid value for flag:
%%d\",\n";
> +      pr "               \"%s\", %s);\n" n n;
> +      pr "    ret = %s;\n" value;
> +      pr "    goto out;\n";
Some of the checks in lib/rw.c are now unreachable with this in place.
Is it worth simplifying that?  (But not all of them - there are still
checks that depend on runtime values, such as nbd_pread accepting _DF
only if the server advertises it after the client requests structured
replies).  Also, this lets us pass all four existing command flags to
all commands that accept an OFlags parameter, even though none of the
commands accept all flags at once - the real protection being added here
is the check for completely unrecognized flags.

But the changes here look reasonable. ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Sat, Aug 10, 2019 at 04:38:20PM -0500, Eric Blake
wrote:
> On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
> > Generate checks that no unknown (at the time of compilation) flags are
> > passed to Flags or OFlags parameters.
> > ---
> >  generator/generator | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> 
> We may still want to introduce a testing mode where you can use libnbd
> to drive unknown flags to a server that understands them, for
> experimenting with protocol extensions not yet included in the NBD
> specification. But if we ever do add such a mode, it shouldn't be hard
> to make this sanity checking conditional on that mode.
> 
> > 
> > diff --git a/generator/generator b/generator/generator
> > index 96d1148..a6aea26 100755
> > --- a/generator/generator
> > +++ b/generator/generator
> > @@ -3689,6 +3689,19 @@ let generate_lib_api_c () > >      );
> >  
> >      (* Check parameters are valid. *)
> > +    let print_flags_check n { flag_prefix; flags } > > +     
let value = match errcode with
> > +        | Some value -> value
> > +        | None -> assert false in
> > +      let mask = List.fold_left (lor) 0 (List.map snd flags) in
> > +      pr "  if ((%s & ~%d) != 0) {\n" n mask;
> > +      pr "    set_error (EINVAL, \"%%s: invalid value for
flag: %%d\",\n";
> > +      pr "               \"%s\", %s);\n" n n;
> > +      pr "    ret = %s;\n" value;
> > +      pr "    goto out;\n";
> 
> Some of the checks in lib/rw.c are now unreachable with this in place.
> Is it worth simplifying that?  (But not all of them - there are still
> checks that depend on runtime values, such as nbd_pread accepting _DF
> only if the server advertises it after the client requests structured
> replies).  Also, this lets us pass all four existing command flags to
> all commands that accept an OFlags parameter, even though none of the
> commands accept all flags at once - the real protection being added here
> is the check for completely unrecognized flags.
Right - this doesn't replace fine-grained checks in the functions
themselves.  I'll have a look at lib/rw.c and see what checks might be
removed.

Rich.
> But the changes here look reasonable. ACK.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Sun, Aug 11, 2019 at 09:26:14AM +0100, Richard W.M. Jones
wrote:
> On Sat, Aug 10, 2019 at 04:38:20PM -0500, Eric Blake wrote:
> > Some of the checks in lib/rw.c are now unreachable with this in place.
> > Is it worth simplifying that?  (But not all of them - there are still
> > checks that depend on runtime values, such as nbd_pread accepting _DF
> > only if the server advertises it after the client requests structured
> > replies).  Also, this lets us pass all four existing command flags to
> > all commands that accept an OFlags parameter, even though none of the
> > commands accept all flags at once - the real protection being added
here
> > is the check for completely unrecognized flags.
> 
> Right - this doesn't replace fine-grained checks in the functions
> themselves.  I'll have a look at lib/rw.c and see what checks might be
> removed.
Hmm - I think all the flag tests in lib/rw.c are still necessary ..?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW