Commit 0f1378b6 authored by Niels Möller's avatar Niels Möller
Browse files

General cleanup and (minor) fixes of some old FIXME:s.

Rev: src/channel.c:1.80
Rev: src/channel.h:1.57
Rev: src/channel_commands.c:1.19
Rev: src/channel_commands.h:1.14
Rev: src/client_keyexchange.c:1.42
Rev: src/client_password.c:1.9
Rev: src/connection.h:1.49
Rev: src/connection_commands.c:1.26
Rev: src/daemon.c:1.6
Rev: src/disconnect.c:1.19
Rev: src/dsa.c:1.13
Rev: src/format.c:1.37
Rev: src/gateway_channel.c:1.2
Rev: src/io.c:1.103
Rev: src/io_commands.c:1.33
Rev: src/keyexchange.c:1.63
Rev: src/keyexchange.h:1.43
Rev: src/lsh.c:1.106
Rev: src/lsh_proxy.c:1.11
Rev: src/lsh_writekey.c:1.19
Rev: src/lshd.c:1.88
Rev: src/pkcs5.c:1.3
Rev: src/proxy.c:1.7
Rev: src/read_packet.c:1.47
Rev: src/reaper.c:1.13
Rev: src/server_keyexchange.c:1.37
Rev: src/server_publickey.c:1.14
Rev: src/server_session.c:1.38
Rev: src/server_userauth.h:1.10
Rev: src/sexp_streamed_parser.c:1.23
Rev: src/sexp_test.c:1.13
Rev: src/spki.c:1.18
Rev: src/ssh1_fallback.c:1.9
Rev: src/tcpforward.c:1.48
Rev: src/unix_user.c:1.13
parent 2784ca77
......@@ -82,19 +82,22 @@ make_channel_open_exception(UINT32 error_code, const char *msg)
*/
struct lsh_string *format_global_failure(void)
struct lsh_string *
format_global_failure(void)
{
return ssh_format("%c", SSH_MSG_REQUEST_FAILURE);
}
struct lsh_string *format_global_success(void)
struct lsh_string *
format_global_success(void)
{
return ssh_format("%c", SSH_MSG_REQUEST_SUCCESS);
}
struct lsh_string *format_open_confirmation(struct ssh_channel *channel,
UINT32 channel_number,
const char *format, ...)
struct lsh_string *
format_open_confirmation(struct ssh_channel *channel,
UINT32 channel_number,
const char *format, ...)
{
va_list args;
UINT32 l1, l2;
......@@ -128,25 +131,29 @@ struct lsh_string *format_open_confirmation(struct ssh_channel *channel,
#undef CONFIRM_ARGS
}
struct lsh_string *format_open_failure(UINT32 channel, UINT32 reason,
const char *msg, const char *language)
struct lsh_string *
format_open_failure(UINT32 channel, UINT32 reason,
const char *msg, const char *language)
{
return ssh_format("%c%i%i%z%z", SSH_MSG_CHANNEL_OPEN_FAILURE,
channel, reason, msg, language);
}
struct lsh_string *format_channel_success(UINT32 channel)
struct lsh_string *
format_channel_success(UINT32 channel)
{
return ssh_format("%c%i", SSH_MSG_CHANNEL_SUCCESS, channel);
}
struct lsh_string *format_channel_failure(UINT32 channel)
struct lsh_string *
format_channel_failure(UINT32 channel)
{
return ssh_format("%c%i", SSH_MSG_CHANNEL_FAILURE, channel);
}
struct lsh_string *prepare_window_adjust(struct ssh_channel *channel,
UINT32 add)
struct lsh_string *
prepare_window_adjust(struct ssh_channel *channel,
UINT32 add)
{
channel->rec_window_size += add;
......@@ -254,7 +261,8 @@ make_exc_finish_channel_handler(struct ssh_connection *connection,
/* Arbitrary limit */
#define MAX_CHANNELS (1L<<17)
struct channel_table *make_channel_table(void)
struct channel_table *
make_channel_table(void)
{
NEW(channel_table, table);
......@@ -285,7 +293,8 @@ struct channel_table *make_channel_table(void)
/* NOTE: This function returns locally chosen channel numbers, which
* are always small integers. So there's no problem fitting them in
* a signed int. */
int alloc_channel(struct channel_table *table)
int
alloc_channel(struct channel_table *table)
{
UINT32 i;
......@@ -334,7 +343,8 @@ int alloc_channel(struct channel_table *table)
return i;
}
void dealloc_channel(struct channel_table *table, int i)
void
dealloc_channel(struct channel_table *table, int i)
{
assert(i >= 0);
assert( (unsigned) i < table->used_channels);
......@@ -376,7 +386,7 @@ register_channel(struct ssh_connection *connection,
verbose("Registering local channel %i.\n",
local_channel_number);
/* FIXME: Is this the right place to install this exception handler? */
/* NOTE: Is this the right place to install this exception handler? */
channel->e =
make_exc_finish_channel_handler(connection,
local_channel_number,
......@@ -408,8 +418,8 @@ lookup_channel_reserved(struct channel_table *table, UINT32 i)
}
/* FIXME: It seems suboptimal to send a window adjust message for *every* write that we do.
* A better scheme might be as follows:
/* FIXME: It seems suboptimal to send a window adjust message for
* *every* write that we do. A better scheme might be as follows:
*
* Delay window adjust messages, keeping track of both the locally
* maintained window size, which is updated after each write, and the
......@@ -417,7 +427,8 @@ lookup_channel_reserved(struct channel_table *table, UINT32 i)
* between these two values gets large enough (say, larger than one
* half or one third of the maximum window size), we send a
* window_adjust message to sync them. */
static void adjust_rec_window(struct flow_controlled *f, UINT32 written)
static void
adjust_rec_window(struct flow_controlled *f, UINT32 written)
{
CAST_SUBTYPE(ssh_channel, channel, f);
......@@ -949,8 +960,9 @@ make_channel_open_continuation(struct ssh_connection *connection,
(remote_channel_number . UINT32)))
*/
static void do_exc_channel_open_handler(struct exception_handler *s,
const struct exception *e)
static void
do_exc_channel_open_handler(struct exception_handler *s,
const struct exception *e)
{
CAST(exc_channel_open_handler, self, s);
......@@ -995,9 +1007,10 @@ make_exc_channel_open_handler(struct ssh_connection *connection,
return &self->super;
}
static void do_channel_open(struct packet_handler *c UNUSED,
struct ssh_connection *connection,
struct lsh_string *packet)
static void
do_channel_open(struct packet_handler *c UNUSED,
struct ssh_connection *connection,
struct lsh_string *packet)
{
struct simple_buffer buffer;
unsigned msg_number;
......@@ -1121,8 +1134,6 @@ do_window_adjust(struct packet_handler *closure UNUSED,
}
else
{
/* FIXME: What to do now? Should unknown channel numbers be
* ignored silently? */
werror("SSH_MSG_CHANNEL_WINDOW_ADJUST on nonexistant or closed "
"channel %i\n", channel_number);
PROTOCOL_ERROR(connection->e, "Unexpected CHANNEL_WINDOW_ADJUST");
......@@ -1338,9 +1349,11 @@ do_channel_eof(struct packet_handler *closure UNUSED,
if (channel->eof)
CHANNEL_EOF(channel);
else
/* FIXME: What is a reasonable default behaviour?
* Closing the channel may be the right thing to do. */
channel_close(channel);
{
/* By default, close the channel. */
debug("No CHANNEL_EOF handler. Closing.\n");
channel_close(channel);
}
}
}
else
......@@ -1702,14 +1715,16 @@ COMMAND_SIMPLE(connection_service_command)
return a;
}
struct lsh_string *format_channel_close(struct ssh_channel *channel)
struct lsh_string *
format_channel_close(struct ssh_channel *channel)
{
return ssh_format("%c%i",
SSH_MSG_CHANNEL_CLOSE,
channel->channel_number);
}
void channel_close(struct ssh_channel *channel)
void
channel_close(struct ssh_channel *channel)
{
static const struct exception finish_exception =
STATIC_EXCEPTION(EXC_FINISH_CHANNEL, "Closing channel");
......@@ -1727,14 +1742,16 @@ void channel_close(struct ssh_channel *channel)
}
}
struct lsh_string *format_channel_eof(struct ssh_channel *channel)
struct lsh_string *
format_channel_eof(struct ssh_channel *channel)
{
return ssh_format("%c%i",
SSH_MSG_CHANNEL_EOF,
channel->channel_number);
}
void channel_eof(struct ssh_channel *channel)
void
channel_eof(struct ssh_channel *channel)
{
if (! (channel->flags &
(CHANNEL_SENT_EOF | CHANNEL_SENT_CLOSE | CHANNEL_RECEIVED_CLOSE)))
......@@ -1753,7 +1770,8 @@ void channel_eof(struct ssh_channel *channel)
}
}
void init_channel(struct ssh_channel *channel)
void
init_channel(struct ssh_channel *channel)
{
/* channel->super.handler = do_read_channel; */
channel->write = NULL;
......@@ -1777,8 +1795,9 @@ void init_channel(struct ssh_channel *channel)
object_queue_init(&channel->active_requests);
}
struct lsh_string *channel_transmit_data(struct ssh_channel *channel,
struct lsh_string *data)
struct lsh_string *
channel_transmit_data(struct ssh_channel *channel,
struct lsh_string *data)
{
assert(data->length <= channel->send_window_size);
assert(data->length <= channel->send_max_packet);
......@@ -1790,9 +1809,10 @@ struct lsh_string *channel_transmit_data(struct ssh_channel *channel,
data);
}
struct lsh_string *channel_transmit_extended(struct ssh_channel *channel,
UINT32 type,
struct lsh_string *data)
struct lsh_string *
channel_transmit_extended(struct ssh_channel *channel,
UINT32 type,
struct lsh_string *data)
{
assert(data->length <= channel->send_window_size);
assert(data->length <= channel->send_max_packet);
......@@ -1931,11 +1951,7 @@ make_channel_read_stderr(struct ssh_channel *channel)
(vars
(channel object ssh_channel))) */
/* Close callback for files we are reading from.
*
* FIXME: I don't know how we should catch POLLERR on files we read;
* perhaps we need this callback, or perhaps we'll install an
* i/o-exception handler do the work. */
/* Close callback for files we are reading from. */
static void
channel_read_close_callback(struct close_callback *c, int reason)
......@@ -1969,7 +1985,12 @@ make_channel_read_close_callback(struct ssh_channel *channel)
* Primarily used for write fd:s that the channel is fed into.
*
* FIXME: Ideally, I'd like to pass something like broken pipe to the
* other end, on write errors, but I don't see how to do that. */
* other end, on write errors, but I don't see how to do that.
*
* NOTE: This isn't used by tcpforward channels. But that is not a big
* problem, because there is only one fd involved. Any error (on
* either read or write) will close that fd, and then the
* channel_read_close_callback will close the channel. */
/* GABA:
(class
......
......@@ -107,10 +107,6 @@
; sources.
(sources simple int)
; FIXME: What about return values from these functions? A
; channel may fail to process it's data. Is there some way to
; propagate a channel broken message to the other end?
; Type is CHANNEL_DATA or CHANNEL_STDERR_DATA
(receive method void "int type" "struct lsh_string *data")
......@@ -118,8 +114,8 @@
; Implies that the send_window_size is non-zero.
(send_adjust method void "UINT32 increment")
; Called when the channel is closed
; FIXME: Is this needed for anything?
; Called when the channel is closed.
; Used by client_session and gateway_channel.
(close method void)
; Called when eof is received on the channel (or when it is
......@@ -167,13 +163,7 @@
/* GABA:
(class
(name channel_table)
;; (super exception_handler)
(vars
; FIXME: This is relevant only for the server side. It's
; probably better to store this in the connection struct.
;; uid_t user; ; Authenticated user
; Channels are indexed by local number
(channels space (object ssh_channel) used_channels)
......
......@@ -36,10 +36,11 @@
#include <assert.h>
void do_channel_open_command(struct command *s,
struct lsh_object *x,
struct command_continuation *c,
struct exception_handler *e)
void
do_channel_open_command(struct command *s,
struct lsh_object *x,
struct command_continuation *c,
struct exception_handler *e)
{
CAST_SUBTYPE(channel_open_command, self, s);
CAST(ssh_connection, connection, x);
......@@ -77,12 +78,11 @@ void do_channel_open_command(struct command *s,
}
}
/* FIXME: Where should we use the passed in exception handler, and when should we use the one
* in the connection struct? */
void do_channel_request_command(struct command *s,
struct lsh_object *x,
struct command_continuation *c,
struct exception_handler *e)
void
do_channel_request_command(struct command *s,
struct lsh_object *x,
struct command_continuation *c,
struct exception_handler *e)
{
CAST_SUBTYPE(channel_request_command, self, s);
CAST_SUBTYPE(ssh_channel, channel, x);
......@@ -97,10 +97,11 @@ void do_channel_request_command(struct command *s,
A_WRITE(channel->write, request);
}
void do_channel_global_command(struct command *s,
struct lsh_object *x,
struct command_continuation *c,
struct exception_handler *e)
void
do_channel_global_command(struct command *s,
struct lsh_object *x,
struct command_continuation *c,
struct exception_handler *e)
{
CAST_SUBTYPE(global_request_command, self, s);
CAST_SUBTYPE(ssh_connection, connection, x);
......
......@@ -118,7 +118,6 @@ do_install_channel_open_handler(struct collect_info_2 *info,
struct lsh_object *a,
struct lsh_object *b);
/* FIXME: This doesn't work */
#define STATIC_INSTALL_GLOBAL_HANDLER(atom) \
{ STATIC_COLLECT_2_FINAL(do_install_global_request_handler),(atom) }
......
......@@ -80,8 +80,8 @@ do_handle_dh_reply(struct packet_handler *c,
NULL, closure->dh.server_key);
if (!v)
/* FIXME: Use a more appropriate error code? */
{
/* FIXME: Use a more appropriate error code? */
disconnect_kex_failed(connection, "Bad server host key\r\n");
return;
}
......@@ -97,7 +97,7 @@ do_handle_dh_reply(struct packet_handler *c,
if (!dh_verify_server_msg(&closure->dh, v))
{
/* FIXME: Same here */
/* FIXME: Use a more appropriate error code? */
disconnect_kex_failed(connection, "Invalid server signature\r\n");
return;
}
......
......@@ -109,7 +109,8 @@ static int echo_off(int fd)
}
/* FIXME: Perhaps it is better to avoid using stdio functions? */
struct lsh_string *read_password(int max_length, struct lsh_string *prompt)
struct lsh_string *
read_password(int max_length, struct lsh_string *prompt)
{
int fd;
FILE *tty;
......
......@@ -65,8 +65,6 @@
(super abstract_write)
(vars
; Where to pass errors
; FIXME: Is this the right place, or should the exception handler be passed
; to each packet handler?
(e object exception_handler)
; Sent and received version strings
......
......@@ -240,8 +240,6 @@ do_line(struct line_handler **h,
werror("Unsupported protocol version: %ps\n",
length, line);
/* FIXME: Clean up properly */
KILL(closure);
*h = NULL;
EXCEPTION_RAISE(connection->e,
......
......@@ -230,7 +230,8 @@ int daemon_started_by_init(void)
/* FIXME: Do we need to detect if the socket is listening or connected
* to a peer? */
int daemon_started_by_inetd(void)
int
daemon_started_by_inetd(void)
{
int optval;
socklen_t optlen = sizeof(optval);
......@@ -239,7 +240,8 @@ int daemon_started_by_inetd(void)
}
/* Disable core files */
int daemon_disable_core(void)
int
daemon_disable_core(void)
{
struct rlimit limit = { 0, 0 };
......
......@@ -46,9 +46,13 @@ do_disconnect(struct packet_handler *closure UNUSED,
{
struct simple_buffer buffer;
unsigned msg_number;
UINT32 length;
UINT32 reason;
UINT32 length;
const UINT8 *msg;
const UINT8 *language;
UINT32 language_length;
static const struct exception disconnect_exception =
STATIC_EXCEPTION(EXC_FINISH_IO, "Received disconnect message.");
......@@ -59,7 +63,8 @@ do_disconnect(struct packet_handler *closure UNUSED,
&& (msg_number == SSH_MSG_DISCONNECT)
&& (parse_uint32(&buffer, &reason))
&& (parse_string(&buffer, &length, &msg))
/* FIXME: Language tag is ignored */ )
&& (parse_string(&buffer, &language_length, &language))
&& parse_eod(&buffer))
{
/* FIXME: Display a better message */
werror("Disconnect for reason %i\n%ups\n", reason, length, msg);
......@@ -69,9 +74,6 @@ do_disconnect(struct packet_handler *closure UNUSED,
lsh_string_free(packet);
/* FIXME: Mark the file as closed, somehow (probably a variable in
* the write buffer) */
EXCEPTION_RAISE(connection->e, &disconnect_exception);
}
......
......@@ -276,7 +276,7 @@ generic_dsa_verify(struct dsa_public *key,
/* Compute w = s^-1 (mod q) */
mpz_init(w);
/* FIXME: mpz_invert generates negative inverses. Is this a problem? */
/* NOTE: mpz_invert somtimes generates negative inverses. */
if (!mpz_invert(w, s, key->q))
{
werror("generic_dsa_verify: s non-invertible.\n");
......@@ -572,8 +572,8 @@ make_ssh_dss_verifier(UINT32 public_length,
}
if (!parse_dsa_public(&buffer, &res->public))
/* FIXME: Perhaps do some more sanity checks? */
{
/* FIXME: Perhaps do some more sanity checks? */
KILL(res);
return NULL;
}
......
......@@ -655,32 +655,10 @@ lsh_string_eq_l(const struct lsh_string *a,
&& !memcmp(a->data, b, length));
}
int lsh_string_prefixp(const struct lsh_string *prefix,
const struct lsh_string *s)
int
lsh_string_prefixp(const struct lsh_string *prefix,
const struct lsh_string *s)
{
return ( (prefix->length <= s->length)
&& !memcmp(prefix->data, s->data, prefix->length));
}
#if 0
/* FIXME: These functions don't really belong here */
int lsh_string_cmp(const struct lsh_string *a, const struct lsh_string *b)
{
if (a->length < b->length)
return -1;
else if (a->length > b->length)
return 1;
else
return memcmp(a->data, b->data, a->length);
}
int lsh_string_cmp_l(const struct lsh_string *a, UINT32 length, const UINT8 *b)
{
if (a->length < length)
return -1;
else if (a->length > length)
return 1;
else
return memcmp(a->data, b, length);
}
#endif
......@@ -43,14 +43,14 @@
* Chaining happens as follows:
*
* 1. First a CHANNEL_OPEN request is received on one connection, and
* an channel object is created. We refer to this object as the
* a channel object is created. We refer to this object as the
* _originating_ channel.
*
* 2. Next, we send a similar CHANNEL_OPEN request on some other
* connection, and create a channel object referred to as the
* _target_ channel.
*
* 3. When we receive a reply to the CHANNEL_OPEN request send in (2),
* 3. When we receive a reply to the CHANNEL_OPEN request sent in (2),
* we chain the two channel objects together, and reply to the
* CHANNEL_OPEN request we received in (1). */
......
......@@ -127,9 +127,6 @@ int io_iter(struct io_backend *b)
if (fd->write_close)
FD_WRITE_CLOSE(fd);
/* FIXME: The value returned from the close
* callback could be used to choose an exit code.
* */
if (fd->close_callback)
{
CLOSE_CALLBACK(fd->close_callback, fd->close_reason);
......@@ -144,10 +141,6 @@ int io_iter(struct io_backend *b)
continue;
}
/* FIXME: nfds should probably include only fd:s that we are
* interested in reading or writing. However, that makes the
* mapping from struct pollfd to struct lsh_fd a little more
* difficult. */
if (fd->want_read || fd->want_write)
nfds++;
......@@ -309,7 +302,6 @@ int io_iter(struct io_backend *b)
}
/* FIXME: Perhaps this function should return a suitable exit code? */
void io_run(struct io_backend *b)
{
struct sigaction pipe;
......@@ -371,9 +363,24 @@ do_buffered_read(struct io_callback *s,
{
UINT32 done;
/* FIXME: What to do if want_read is false? */
assert(fd->want_read);
assert(self->handler);
/* FIXME: What to do if want_read is false?
* To improve the connection_lock() mechanism,
* it must be possible to temporarily stop reading, which means that
* fd->want_read has to be cleared.
*
* But when doing this, we have to keep the data that we
* have read, some of which is buffered here, on the stack,
* and the rest inside the read-handler.
*
* There are two alternatives: Save our buffer here, or
* continue looping, letting the read-handler process it
* into packets. In the latter case, the ssh_connection
* could keep a queue of waiting packets, but it would still
* have to clear the want_read flag, to prevent that queue
* from growing arbitrarily large.
*/
assert(fd->want_read); assert(self->handler);
/* NOTE: This call may replace self->handler */
done = READ_HANDLER(self->handler, left, buffer);
......@@ -675,6 +682,8 @@ static void do_kill_fd(struct resource *r)
/* NOTE: We use the zero close reason for files killed this way.
* Close callbacks are still called, but they should probably not do
* anything if reason == 0. */
/* FIXME: Should we use close_fd() or close_fd_nicely() ? */
if (r->alive)
close_fd(fd, 0);
}
......@@ -723,7 +732,7 @@ static void init_file(struct io_backend *b, struct lsh_fd *f, int fd,
b->files = f;
}
#if 0
/* Blocking read from a file descriptor (i.e. don't use the backend).
* The fd should *not* be in non-blocking mode. */
......@@ -772,7 +781,7 @@ int blocking_read(int fd, struct read_handler *handler)
close(fd);
return !handler;
}
#endif
/* These functions are used by werror() and friends */
......@@ -879,123 +888,6 @@ int get_portno(const char *service, const char *protocol)
}
}
#if 0
/*
* Fill in ADDR from HOST, SERVICE and PROTOCOL.
* Supplying a null pointer for HOST means use INADDR_ANY.
* Otherwise HOST is an numbers-and-dots ip-number or a dns name.
*
* PROTOCOL can be tcp or udp.
*
* Supplying a null pointer for SERVICE, means use port 0, i.e. no port.
*
* Returns zero on errors, 1 if everything is ok.
*/
int
get_inaddr(struct sockaddr * addr,
const char * host,
const char * service,
const char * protocol)
{
/* HERE!!! IPv6 */
memset(addr, 0, sizeof *addr);
addr->sin_family = AF_INET;
/*
* Set host part of ADDR
*/
if (host == NULL)
addr->sin_addr.s_addr = INADDR_ANY;