Skip to content
Snippets Groups Projects
  1. Mar 10, 2020
    • Simon Tatham's avatar
      Change vtable defs to use C99 designated initialisers. · b4e1bca2
      Simon Tatham authored
      This is a sweeping change applied across the whole code base by a spot
      of Emacs Lisp. Now, everywhere I declare a vtable filled with function
      pointers (and the occasional const data member), all the members of
      the vtable structure are initialised by name using the '.fieldname =
      value' syntax introduced in C99.
      
      We were already using this syntax for a handful of things in the new
      key-generation progress report system, so it's not new to the code
      base as a whole.
      
      The advantage is that now, when a vtable only declares a subset of the
      available fields, I can initialise the rest to NULL or zero just by
      leaving them out. This is most dramatic in a couple of the outlying
      vtables in things like psocks (which has a ConnectionLayerVtable
      containing only one non-NULL method), but less dramatically, it means
      that the new 'flags' field in BackendVtable can be completely left out
      of every backend definition except for the SUPDUP one which defines it
      to a nonzero value. Similarly, the test_for_upstream method only used
      by SSH doesn't have to be mentioned in the rest of the backends;
      network Plugs for listening sockets don't have to explicitly null out
      'receive' and 'sent', and vice versa for 'accepting', and so on.
      
      While I'm at it, I've normalised the declarations so they don't use
      the unnecessarily verbose 'struct' keyword. Also a handful of them
      weren't const; now they are.
      b4e1bca2
  2. Feb 06, 2019
  3. Feb 04, 2019
    • Simon Tatham's avatar
      Stop using unqualified {GET,PUT}_32BIT. · acc21c4c
      Simon Tatham authored
      Those were a reasonable abbreviation when the code almost never had to
      deal with little-endian numbers, but they've crept into enough places
      now (e.g. the ECC formatting) that I think I'd now prefer that every
      use of the integer read/write macros was clearly marked with its
      endianness.
      
      So all uses of GET_??BIT and PUT_??BIT are now qualified. The special
      versions in x11fwd.c, which used variable endianness because so does
      the X11 protocol, are suffixed _X11 to make that clear, and where that
      pushed line lengths over 80 characters I've taken the opportunity to
      name a local variable to remind me of what that extra parameter
      actually does.
      acc21c4c
  4. Nov 03, 2018
    • Simon Tatham's avatar
      Convert a lot of 'int' variables to 'bool'. · 3214563d
      Simon Tatham authored
      My normal habit these days, in new code, is to treat int and bool as
      _almost_ completely separate types. I'm still willing to use C's
      implicit test for zero on an integer (e.g. 'if (!blob.len)' is fine,
      no need to spell it out as blob.len != 0), but generally, if a
      variable is going to be conceptually a boolean, I like to declare it
      bool and assign to it using 'true' or 'false' rather than 0 or 1.
      
      PuTTY is an exception, because it predates the C99 bool, and I've
      stuck to its existing coding style even when adding new code to it.
      But it's been annoying me more and more, so now that I've decided C99
      bool is an acceptable thing to require from our toolchain in the first
      place, here's a quite thorough trawl through the source doing
      'boolification'. Many variables and function parameters are now typed
      as bool rather than int; many assignments of 0 or 1 to those variables
      are now spelled 'true' or 'false'.
      
      I managed this thorough conversion with the help of a custom clang
      plugin that I wrote to trawl the AST and apply heuristics to point out
      where things might want changing. So I've even managed to do a decent
      job on parts of the code I haven't looked at in years!
      
      To make the plugin's work easier, I pushed platform front ends
      generally in the direction of using standard 'bool' in preference to
      platform-specific boolean types like Windows BOOL or GTK's gboolean;
      I've left the platform booleans in places they _have_ to be for the
      platform APIs to work right, but variables only used by my own code
      have been converted wherever I found them.
      
      In a few places there are int values that look very like booleans in
      _most_ of the places they're used, but have a rarely-used third value,
      or a distinction between different nonzero values that most users
      don't care about. In these cases, I've _removed_ uses of 'true' and
      'false' for the return values, to emphasise that there's something
      more subtle going on than a simple boolean answer:
       - the 'multisel' field in dialog.h's list box structure, for which
         the GTK front end in particular recognises a difference between 1
         and 2 but nearly everything else treats as boolean
       - the 'urgent' parameter to plug_receive, where 1 vs 2 tells you
         something about the specific location of the urgent pointer, but
         most clients only care about 0 vs 'something nonzero'
       - the return value of wc_match, where -1 indicates a syntax error in
         the wildcard.
       - the return values from SSH-1 RSA-key loading functions, which use
         -1 for 'wrong passphrase' and 0 for all other failures (so any
         caller which already knows it's not loading an _encrypted private_
         key can treat them as boolean)
       - term->esc_query, and the 'query' parameter in toggle_mode in
         terminal.c, which _usually_ hold 0 for ESC[123h or 1 for ESC[?123h,
         but can also hold -1 for some other intervening character that we
         don't support.
      
      In a few places there's an integer that I haven't turned into a bool
      even though it really _can_ only take values 0 or 1 (and, as above,
      tried to make the call sites consistent in not calling those values
      true and false), on the grounds that I thought it would make it more
      confusing to imply that the 0 value was in some sense 'negative' or
      bad and the 1 positive or good:
       - the return value of plug_accepting uses the POSIXish convention of
         0=success and nonzero=error; I think if I made it bool then I'd
         also want to reverse its sense, and that's a job for a separate
         piece of work.
       - the 'screen' parameter to lineptr() in terminal.c, where 0 and 1
         represent the default and alternate screens. There's no obvious
         reason why one of those should be considered 'true' or 'positive'
         or 'success' - they're just indices - so I've left it as int.
      
      ssh_scp_recv had particularly confusing semantics for its previous int
      return value: its call sites used '<= 0' to check for error, but it
      never actually returned a negative number, just 0 or 1. Now the
      function and its call sites agree that it's a bool.
      
      In a couple of places I've renamed variables called 'ret', because I
      don't like that name any more - it's unclear whether it means the
      return value (in preparation) for the _containing_ function or the
      return value received from a subroutine call, and occasionally I've
      accidentally used the same variable for both and introduced a bug. So
      where one of those got in my way, I've renamed it to 'toret' or 'retd'
      (the latter short for 'returned') in line with my usual modern
      practice, but I haven't done a thorough job of finding all of them.
      
      Finally, one amusing side effect of doing this is that I've had to
      separate quite a few chained assignments. It used to be perfectly fine
      to write 'a = b = c = TRUE' when a,b,c were int and TRUE was just a
      the 'true' defined by stdbool.h, that idiom provokes a warning from
      gcc: 'suggest parentheses around assignment used as truth value'!
      3214563d
    • Simon Tatham's avatar
      Adopt C99 <stdbool.h>'s true/false. · a6f1709c
      Simon Tatham authored
      This commit includes <stdbool.h> from defs.h and deletes my
      traditional definitions of TRUE and FALSE, but other than that, it's a
      100% mechanical search-and-replace transforming all uses of TRUE and
      FALSE into the C99-standardised lowercase spellings.
      
      No actual types are changed in this commit; that will come next. This
      is just getting the noise out of the way, so that subsequent commits
      can have a higher proportion of signal.
      a6f1709c
  5. Oct 21, 2018
    • Simon Tatham's avatar
      Server prep: parse a lot of new channel requests. · 9fe719f4
      Simon Tatham authored
      ssh2connection.c now knows how to unmarshal the message formats for
      all the channel requests we'll need to handle when we're the server
      and a client sends them. Each one is translated into a call to a new
      method in the Channel vtable, which is implemented by a trivial
      'always fail' routine in every channel type we know about so far.
      9fe719f4
    • Simon Tatham's avatar
      Allow channels not to close immediately after two EOFs. · d3a9142d
      Simon Tatham authored
      Some kinds of channel, even after they've sent EOF in both directions,
      still have something to do before they initiate the CLOSE mechanism
      and wind up the channel completely. For example, a session channel
      with a subprocess running inside it will want to be sure to send the
      "exit-status" or "exit-signal" notification, even if that happens
      after bidirectional EOF of the data channels.
      
      Previously, the SSH-2 connection layer had the standard policy that
      once EOF had been both sent and received, it would start the final
      close procedure. There's a method chan_want_close() by which a Channel
      could vary this policy in one direction, by indicating that it wanted
      the close procedure to commence after EOF was sent in only one
      direction. Its parameters are a pair of booleans saying whether EOF
      has been sent, and whether it's been received.
      
      Now chan_want_close can vary the policy in the other direction as
      well: if it returns FALSE even when _both_ parameters are true, the
      connection layer will honour that, and not send CHANNEL_CLOSE. If it
      does that, the Channel is responsible for indicating when it _does_
      want close later, by calling sshfwd_initiate_close.
      d3a9142d
    • Simon Tatham's avatar
      Move channel-opening logic out into subroutines. · d1cd8b25
      Simon Tatham authored
      Each of the new subroutines corresponds to one of the channel types
      for which we know how to parse a CHANNEL_OPEN, and has a collection of
      parameters corresponding to the fields of that message structure.
      ssh2_connection_filter_queue now confines itself to parsing the
      message, calling one of those functions, and constructing an
      appropriate reply message if any.
      d1cd8b25
    • Simon Tatham's avatar
      Devolve channel-request handling to Channel vtable. · 2339efcd
      Simon Tatham authored
      Instead of the central code in ssh2_connection_filter_queue doing both
      the job of parsing the channel request and deciding whether it's
      acceptable, each Channel vtable now has a method for every channel
      request type we recognise.
      2339efcd
  6. Oct 06, 2018
    • Simon Tatham's avatar
      Rename FROMFIELD to 'container_of'. · 9396fcc9
      Simon Tatham authored
      Ian Jackson points out that the Linux kernel has a macro of this name
      with the same purpose, and suggests that it's a good idea to use the
      same name as they do, so that at least some people reading one code
      base might recognise it from the other.
      
      I never really thought very hard about what order FROMFIELD's
      parameters should go in, and therefore I'm pleasantly surprised to
      find that my order agrees with the kernel's, so I don't have to
      permute every call site as part of making this change :-)
      9396fcc9
  7. Sep 24, 2018
    • Simon Tatham's avatar
      Fix spurious EOF in agent forwarding! · 56bf65ef
      Simon Tatham authored
      Commit 6a8b9d38, which created the Channel vtable and moved the agent
      forwarding implementation of it out into agentf.c, managed to set the
      rcvd_eof flag to TRUE in agentf_new(), meaning that we behave exactly
      as if the first agent request was followed by an incoming EOF.
      56bf65ef
  8. Sep 20, 2018
    • Simon Tatham's avatar
      Put a layer of abstraction in front of struct ssh_channel. · aa08e6ca
      Simon Tatham authored
      Clients outside ssh.c - all implementations of Channel - will now not
      see the ssh_channel data type itself, but only a subobject of the
      interface type SshChannel. All the sshfwd_* functions have become
      methods in that interface type's vtable (though, wrapped in the usual
      kind of macros, the call sites look identical).
      
      This paves the way for me to split up the SSH-1 and SSH-2 connection
      layers and have each one lay out its channel bookkeeping structure as
      it sees fit; as long as they each provide an implementation of the
      sshfwd_ method family, the types behind that need not look different.
      
      A minor good effect of this is that the sshfwd_ methods are no longer
      global symbols, so they don't have to be stubbed in Unix Pageant to
      get it to compile.
      aa08e6ca
    • Simon Tatham's avatar
      Replace enum+union of local channel types with a vtable. · 6a8b9d38
      Simon Tatham authored
      There's now an interface called 'Channel', which handles the local
      side of an SSH connection-layer channel, in terms of knowing where to
      send incoming channel data to, whether to close the channel, etc.
      
      Channel and the previous 'struct ssh_channel' mutually refer. The
      latter contains all the SSH-specific parts, and as much of the common
      logic as possible: in particular, Channel doesn't have to know
      anything about SSH packet formats, or which SSH protocol version is in
      use, or deal with all the fiddly stuff about window sizes - with the
      exception that x11fwd.c's implementation of it does have to be able to
      ask for a small fixed initial window size for the bodgy system that
      distinguishes upstream from downstream X forwardings.
      
      I've taken the opportunity to move the code implementing the detailed
      behaviour of agent forwarding out of ssh.c, now that all of it is on
      the far side of a uniform interface. (This also means that if I later
      implement agent forwarding directly to a Unix socket as an
      alternative, it'll be a matter of changing just the one call to
      agentf_new() that makes the Channel to plug into a forwarding.)
      6a8b9d38
Loading