GSS kex: remove spurious no-op assignment.

Coverity points out that under a carefully checked compound condition,
we assign s->gss_cred_expiry to itself. :-)

Before commit 2ca0070f8 split the SSH code into separate modules, that
assignment read 'ssh->gss_cred_expiry = s->gss_cred_expiry', and the
point was that the value had to be copied out of the private state of
the transport-layer coroutine into the general state of the SSH
protocol as a whole so that ssh2_timer() (or rather, ssh2_gss_update,
called from ssh2_timer) could check it.

But now that timer function is _also_ local to the transport layer,
and shares the transport coroutine's state. So on the one hand, we
could just remove that assignment, folding the two variables into one;
on the other hand, we could reinstate the two variables and the
explicit 'commit' action that copies one into the other. The question
is, which?

Any _successful_ key exchange must have gone through that commit step
of the kex that copied the one into the other. And an unsuccessful key
exchange always aborts the whole SSH connection - there's nothing in
the SSH transport protocol that allows recovering from a failed rekey
by carrying on with the previous session keys. So if I made two
variables, then between key exchanges, they would always have the same
value anyway.

So the only effect of the separation between those variables is
_during_ a GSS key exchange: should the version of gss_cred_expiry
checked by the timer function be the new one, or the old one?

This can only make a difference if a timer goes off _during_ a rekey,
and happens to notice that the GSS credentials have expired. And
actually I think it's mildly _better_ if that checks the new expiry
time rather than the old one - otherwise, the timer routine would set
the flag indicating a need for another rekey, when actually the
currently running one was going to get the job done anyway.

So, in summary, I think conflating those two variables is actually an
improvement to the code. I did it by accident, but if I'd realised, I
would have done the same thing on purpose!

Hence, I've just removed the pointless self-assignment, with no
functional change.
This commit is contained in:
Simon Tatham 2019-05-05 09:41:24 +01:00
Родитель f1fe0c7d8d
Коммит 2b1e0a5e05
1 изменённых файлов: 0 добавлений и 3 удалений

Просмотреть файл

@ -412,9 +412,6 @@ void ssh2kex_coroutine(struct ssh2_transport_state *s, bool *aborted)
data = get_string(pktin);
s->mic.value = (char *)data.ptr;
s->mic.length = data.len;
/* Save expiration time of cred when delegating */
if (s->gss_delegate && s->gss_cred_expiry != GSS_NO_EXPIRATION)
s->gss_cred_expiry = s->gss_cred_expiry;
/* If there's a final token we loop to consume it */
if (get_bool(pktin)) {
data = get_string(pktin);