It tries to use the local username as the remote username if it has no
better ideas, but the presence of Default Settings would defeat this,
even if it had no username set. Reported by Jonathan Amery.
We were allocating a new array in which to make up a random number
every time we went round the loop, and not freeing any of them. Now we
allocate a single array to use for all loop iterations, and clear and
free it properly afterwards.
Patch due to Tim Kosse.
We were checking the return value of CreateThread for validity, but
not keeping it to free afterwards if it _was_ valid. Also, we weren't
closing ctx->event in the valid case either. Patch due to Tim Kosse.
There was an error-handling path testing the wrong variable; an
inappropriate call to ec_point_free in decodepoint() (in fact, that
function always gets passed a pointer to an ec_point structure that's
not a dynamically allocated block at all or not in its own right, so
we should have just cleared its contents without freeing the structure
itself); a missing return on an error path which would have caused the
same structure to be freed a second time; and two missing freebn in
ecdsa_sign.
Patch due to Tim Kosse.
If we use getaddrinfo to translate the source IP address into a
sockaddr, then we need to freeaddrinfo the returned data later. Patch
due to Tim Kosse.
I don't think anyone has ever actually called it that, colloquially
_or_ formally, and if anyone ever did (in a bug report, say) I'd
probably have to stop and think to work out what they meant. It's
universally called Plink, and should be officially so as well :-)
This one spotted in the old-fashioned way, by actually attempting a
Plink raw connection and wondering why it didn't seem to be reading
from standard input! Turns out 'bufsize' is uninitialised until the
first send, which can inhibit any stdin reading if it gets a large
enough nonsense value.
mkfiles.pl was giving a couple of annoying perl warnings, because some
makefile_extra strings were never set by Recipe. We already have the
&def function to convert undefs into "" for this reason, but weren't
using it everywhere. Now I think we are.
I'm not actually sure why we've always had back ends notify ldisc of
changes to echo/edit settings by giving ldisc_send(ldisc,NULL,0,0) a
special meaning, instead of by having a separate dedicated notify
function with its own prototype and parameter set. Coverity's recent
observation that the two kinds of call don't even have the same
requirements on the ldisc (particularly, whether ldisc->term can be
NULL) makes me realise that it's really high time I separated the two
conceptually different operations into actually different functions.
While I'm here, I've renamed the confusing ldisc_update() function
which that special operation ends up feeding to, because it's not
actually a function applying to an ldisc - it applies to a front end.
So ldisc_send(ldisc,NULL,0,0) is now ldisc_echoedit_update(ldisc), and
that in turn figures out the current echo/edit settings before passing
them on to frontend_echoedit_update(). I think that should be clearer.
If a sharing downstream asks for an auth method we don't understand,
we should send them CHANNEL_FAILURE *and then stop processing*. Ahem.
(Spotted while examining this code in the course of Coverity-related
fixes, but not itself a Coverity-found problem.)
If (Msg)WaitForMultipleObjects returns WAIT_TIMEOUT, we expect 'next'
to have been initialised. This can occur without having called
run_timers(), if a toplevel callback was pending, so we can't expect
run_timers to have reliably initialised 'next'.
I'm not actually convinced this could have come up in either of the
affected programs (Windows PSFTP and Plink), due to the list of things
toplevel callbacks are currently used for, but it certainly wants
fixing anyway for the future.
Spotted by Coverity.
Namely, any ldisc that you send actual data through should have a
terminal attached, because the ldisc editing/echoing system is
designed entirely for use with a terminal. The only time you can have
an ldisc with no terminal is when it's only ever used by the backend
to report changes to the front end in edit/echo status, e.g. by Unix
Plink.
Coverity spotted an oddity in ldisc_send which after a while I decided
would never have actually caused a problem, but OTOH I agree that it
was confusing, so now hopefully it's less so.
'p += strcspn' returns p always non-NULL and sometimes pointing at \0,
as opposed to 'p = strchr' which returns p sometimes non-NULL and
never pointing at \0. Test the pointer after the call accordingly.
Thanks, Coverity.
This ought to happen in ssh_do_close alongside the code that shuts
down other local listening things like port forwardings, for the same
obvious reason. In particular, we should get through this _before_ we
put up a modal dialog box telling the user what just went wrong with
the SSH connection, so that further sessions started while that box is
active don't try futilely to connect to the not-really-listening
zombie upstream.
Changing it can't have any useful effect, since we have strictly
enforced that the host key used for rekeys is the same as the first key
exchange since b8e668c.
