[Swan-dev] cur_state and friends [was Re:Fwd: Changes to ref refs/heads/master]

[Andrew Cagney]

On 16 January 2018 at 13:54, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
> [All this is from fallible memory since archaeology is
> labour-intensive.]
>
> | From: Andrew Cagney <andrew.cagney at gmail.com>
>
> | Yea, this is part of a more general problem.  But first some
> | background.  We've always kind of sort of always had the stack:
> |
> | - cur_state | ... | cur_state | cur_connection | cur_from
>
> The original design apparently wasn't robust in the face of 20 years
> of maintenance.  Either a little two intricate or not documented well
> enough.

Likely neither.  No matter how good our, code has this annoying habit
of evolving in ways we don't expect but do work.

Here, it has ended up being used as a stack.  It also isn't that
surprising as the code has a natural flow:

            ip_address old_from = push_cur_from(md->sender);
            process_packet(&md);
            pop_cur_from(old_from);

or:

        so_serial_t old_state = push_cur_state(st);
        struct msg_digest *md = unsuspend_md(st);
        (*cn->pcrc_func)(st, md, &cn->pcrc_pcr);
        pop_cur_state(old_state);

or:

        so_serial_t old_state = push_cur_state(st);
        libreswan_log("XAUTH: #%lu: completed for user '%s' with status %s",
                  xauth->serialno, xauth->ptarg.name,
                  success ? "SUCCESSS" : "FAILURE");
        xauth->callback(st, xauth->ptarg.name, success);
        pop_cur_state(old_state);

or more generally:

      st = find state
      if st != NULL {
          push st
          process st
          pop
      }

and in IKEv2's auth code:

     push parent
     ...
     /* parent authenticated, start on child *.
      push cst
          finish child
      pop cst
    pop parent

(but the last one doesn't look like that, and I'd argue that is one of
the causes of a horrible PPK bug I hacked around)

> There really was no stack.  But there was a priority.  If cur_state was
> set, that was used to prefix logging.  Otherwise, if cur_connection was
> set, that was used to prefix logging.  cur_* were ONLY supposed to be used
> for logging.
>
> set_cur* were meant to be idempotent.  Certainly not a feature of stack
> pushing.  Idempotence makes correctness easier (eg. the change to
> timer_event_cb would not be required).

... or timer_event_cb() could be structured so that it better follows
the above flow

> Some (not many) routines needed to temporarily switch cur_state.  It was
> their job to save the old cur_state value and later restore it.  If this
> were common, a stack would make sense.  With the transaction-processing
> design of Pluto, this was not at all common.
>
> There is a particular hazard with a routine privately saving a copy
> of cur_state.  If that old state were deleted, who would prevent that
> dangling pointer being re-installed as cur_state?  I don't think that
> case could come up, but who knows with all the code changes that have
> happened.

I fixed the case with cur_from.  Since it has a copy of the address it
doesn't matter when MD gets deleted.

> One of my biggest sins was to use cur_state to smuggle the current state
> around some of the crypto code.  It was meant to be a temporary hack until
> I got the next chunk of changes done and committed.  But that didn't
> happen.  I think Andrew fixed that recently with some refactoring.
>
> | - when a state gets deleted, there's general confusion over who should
> | be pushing/popping cur_state
>
> In the original design, only one thing ever deleted a state:
> delete_state().
>
> If cur_state pointed to the state to be deleted, delete_state would
> set it to NULL.  But it didn't do that immediately since some of its
> diagnostics were usefully prefixed.
>
> Same pattern for cur_connection.

While my cur_from code works, I think there's an even simpler solution.

Instead of saving the current from / connection / state, simply
pre-format the prefix and and save that (possibly on a local stack)
(see LSWLOG_STRING()).

This way it doesn't matter if/when state or connection are deleted as
the prefix string is always valid.  Code like this:

    /* without st_connection, st isn't complete */
    /* from here on logging is for the wrong state */
    pop_cur_state(old_serialno);

isn't needed.

> With a stack, things get trickier.  It's nice for things that act like
> brackets dynamically (i.e. in execution) to appear as brackets
> statically (i.e. in the code).  That's not often the case with
> push_cur_state and pop_cur_state.

the above examples were mostly lifted from the code