On Mon, Sep 05, 2022 at 04:35:28PM +0200, Laszlo Ersek wrote:
> On 08/31/22 16:39, Eric Blake wrote:
> > Add a new control knob nbd_set_request_meta_context(), modeled after
> > the existing nbd_set_request_structured_replies(), to make it possible
> > to skip the NBD_OPT_SET_META_CONTEXT half of the two-command sequence
> > currently performed in nbd_opt_go() and nbd_opt_info(). Also add a
> > counterpart nbd_get_request_meta_context() for symmetry.
> >
...
> > +++ b/generator/states-newstyle-opt-meta-context.c
> > @@ -29,9 +29,6 @@ STATE_MACHINE {
> > */
> > assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
> > nbd_internal_reset_size_and_flags (h);
> > - for (i = 0; i < h->meta_contexts.len; ++i)
> > - free (h->meta_contexts.ptr[i].name);
> > - meta_vector_reset (&h->meta_contexts);
> > if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
> > assert (h->opt_mode);
> > assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context));
> > @@ -40,13 +37,19 @@ STATE_MACHINE {
> > else {
> > assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> > opt = NBD_OPT_SET_META_CONTEXT;
> > - if (!h->structured_replies || h->request_meta_contexts.len == 0) {
> > - SET_NEXT_STATE (%^OPT_GO.START);
> > - return 0;
> > + if (h->request_meta) {
> > + for (i = 0; i < h->meta_contexts.len; ++i)
> > + free (h->meta_contexts.ptr[i].name);
> > + meta_vector_reset (&h->meta_contexts);
> > + h->meta_valid = false;
> > }
> > }
> > -
> > - assert (!h->meta_valid);
> > + if (opt != h->opt_current &&
> > + (!h->request_meta || !h->structured_replies ||
> > + h->request_meta_contexts.len == 0)) {
> > + SET_NEXT_STATE (%^OPT_GO.START);
> > + return 0;
> > + }
> >
> > /* Calculate the length of the option request data. */
> > len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries
> > */;
>
> I'm confused about moving the transition to GO.START out of its current
> enclosing block, and then compensating (?) for that unnesting with the
> additional (opt != h->opt_current) restriction.
>
> More precisely, I don't understand how (opt != h->opt_current) could
> ever evaluate to 1.
Good question. Thanks to tweaks I made to 2/12 before pushing that
one (55b09667), in v3 this patch 4/12 will be starting with a better
comment pre-patch:
/* This state group is reached from:
* h->opt_mode == false (h->opt_current == 0):
* nbd_connect_*()
* -> conditionally use SET, next state OPT_GO for NBD_OPT_GO
* h->opt_mode == true (h->opt_current matches calling API):
* nbd_opt_info()
* -> conditionally use SET, next state OPT_GO for NBD_OPT_INFO
* nbd_opt_go()
* -> conditionally use SET, next state OPT_GO for NBD_OPT_GO
* nbd_opt_list_meta_context()
* -> conditionally use LIST, next state NEGOTIATING
*
* For now, we start by unconditionally clearing h->exportsize and friends,
* as well as h->meta_contexts and h->meta_valid.
* If SET is conditional, we skip it if structured replies were
* not negotiated, or if there were no contexts to request.
* SET then manipulates h->meta_contexts, and sets h->meta_valid on success.
* If OPT_GO is later successful, it populates h->exportsize and friends,
* and also sets h->meta_valid if we skipped SET here.
* LIST is conditional, skipped if structured replies were not negotiated.
* There is a callback if and only if the command is LIST.
*/
>
> Here's the code, with this patch applied:
>
> > if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
> > assert (h->opt_mode);
> > assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context));
> > opt = h->opt_current;
> > }
> > else {
> > assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> > opt = NBD_OPT_SET_META_CONTEXT;
And this is the key point - here, we are slamming opt to
SET_META_CONTEXT, while leaving h->opt_current as 0 (nbd_connect_*),
GO, or INFO. h->opt_current won't be SET_META_CONTEXT until later in
the series when the nbd_opt_set_meta_context() API is added; at which
point, the v3 patch will update the comment to match.
> > if (h->request_meta) {
> > for (i = 0; i < h->meta_contexts.len; ++i)
> > free (h->meta_contexts.ptr[i].name);
> > meta_vector_reset (&h->meta_contexts);
> > h->meta_valid = false;
> > }
> > }
> > if (opt != h->opt_current &&
> > (!h->request_meta || !h->structured_replies ||
> > h->request_meta_contexts.len == 0)) {
> > SET_NEXT_STATE (%^OPT_GO.START);
> > return 0;
> > }
>
> My understanding is that we enter this code with either one of two
> possible "h->opt_current" values: NBD_OPT_LIST_META_CONTEXT,
> NBD_OPT_SET_META_CONTEXT.
That's where the confusion stemmed from; without the comment
explaining the 4 possible values of h->opt_current on entry (5 after
nbd_opt_set_meta_context() is introduced later), it's harder to see
why it made more sense to isolate the decision to short-circuit over
to OPT_GO (what the comment calls "conditional SET") separately from
the decision of whether to reset h->meta_contexts (we want the reset
for SET but not for LIST, even if we don't end up sending SET over the
wire).
>
> - In the first case, when "h->opt_current" equals LIST, "opt" at the end
> of the first branch will be LIST (via assignment from
> "h->opt_current"), that is, equal "h->opt_current".
>
> - In the second case (when "h->opt_current" differs from LIST, hence,
> when it equals SET), "opt" at the end of the second branch will be SET
> (after having directly been assigned SET) -- that is, "opt" will equal
> "h->opt_current" *again*.
>
> That suggests the (opt != h->opt_current) condition will never evaluate
> to "true". (And it also suggests that there is no good reason for the
> "opt" local variable to exist...)
>
> Now, in case I'm wrong, and we enter this code with a *third*
> "h->opt_current" value, then we have:
>
> - opt = SET, and
>
> - "h->opt_current" differing from both SET and LIST.
>
> Then we indeed transition to GO.START -- but then, wouldn't the
> following hunk be *simpler*?
For this patch, the following hunk would indeed be less convoluted,
but later in the series when nbd_opt_set_meta_context() is added, I
really did need to rearrange the control flow; whether doing it in
this patch makes the most sense, or deferring the control flow
rearrangement to later would have been wiser, I don't know, but we'll
see if the comments in v3 make it easier to follow.
>
> > diff --git a/generator/states-newstyle-opt-meta-context.c
> > b/generator/states-newstyle-opt-meta-context.c
> > index 5c654543a228..1e31eedd4cef 100644
> > --- a/generator/states-newstyle-opt-meta-context.c
> > +++ b/generator/states-newstyle-opt-meta-context.c
> > @@ -40,14 +40,21 @@ STATE_MACHINE {
> > else {
> > assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> > opt = NBD_OPT_SET_META_CONTEXT;
> > - if (!h->structured_replies || h->request_meta_contexts.len == 0) {
> > +
> > + if (h->request_meta) {
> > + for (i = 0; i < h->meta_contexts.len; ++i)
> > + free (h->meta_contexts.ptr[i].name);
> > + meta_vector_reset (&h->meta_contexts);
> > + h->meta_valid = false;
> > + }
> > +
> > + if (!h->structured_replies ||
> > + !h->request_meta || h->request_meta_contexts.len == 0) {
> > SET_NEXT_STATE (%^OPT_GO.START);
> > return 0;
> > }
> > }
> >
> > - assert (!h->meta_valid);
> > -
> > /* Calculate the length of the option request data. */
> > len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries
> > */;
> > for (i = 0; i < h->request_meta_contexts.len; ++i)
>
> ... FWIW, I think the formulation I'm proposing is correct (unlike the
> patch) even in case we can only enter this section with SET and LIST!
As stated above, in this patch, we can only enter with h->opt_current
== 0, LIST, GO, or INFO. And for what it's worth, in an earlier
iteration of my patch, my logic did look more like your proposed
alternative hunk.
>
> Back to the patch:
>
> On 08/31/22 16:39, Eric Blake wrote:
> > diff --git a/lib/flags.c b/lib/flags.c
> > index 91efc1a..c8c68ea 100644
> > --- a/lib/flags.c
> > +++ b/lib/flags.c
> > @@ -37,7 +37,6 @@ nbd_internal_reset_size_and_flags (struct nbd_handle *h)
> >
> > h->exportsize = 0;
> > h->eflags = 0;
> > - h->meta_valid = false;
> > h->block_minimum = 0;
> > h->block_preferred = 0;
> > h->block_maximum = 0;
>
> Shouldn't we clear "h->meta_valid" in nbd_close() as well?
>
> nbd_close() calls nbd_internal_reset_size_and_flags(), and clears the
> metacontext vector.
Hmm, good question. It is undefined to call nbd_FOO(nbd) if you have
previously called nbd_close(nbd); but to minimize the risk of
nbd_can_meta_context() doing something weird if the UAF hasn't already
hit the worse case of something else clobbering the memory, then yes,
explicitly clearing h->meta_valid during close is probably wise. I'll
add that into v3.
> > +++ b/tests/opt-set-meta
> > @@ -0,0 +1,210 @@
> > +#! /bin/sh
> > +
> > +# opt-set-meta - temporary wrapper script for .libs/opt-set-meta
> > +# Generated by libtool (GNU libtool) 2.4.6
>
> ... Ugh, I initially missed this "generated by" line, and wondered (a)
> why the script read like line noise, (b) why you spent time on ZSH and
> Tru64 (!!!) compatibility, (c) what the script *actually did*.
>
> Did you git-add this (generated) script to the commit by mistake,
> perhaps?
Yep, and I had already mentioned it in my reply to 0/12; but by
failing to also mention it in reply to this message, I've cost you
some review time; sorry about that. At any rate, I've fixed that
already in my tree for v3 to quit adding libtool-generated files. I
don't know if it would have been any easier had it been an actual
binary instead of a libtool script (would git have given me the
one-line 'binary diff' message, or blown up the email into trying to
represent the binary?)
>
> Looks OK to me otherwise.
>
> Thanks,
> Laszlo
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
