While looking for leaks I came across two old packet APIs which are easy
to remove. I'm sending patches for each separately.
First, there is the packet_set_connection(int fd_in, int fd_out)
function in opacket.c
The function relies on a behavior in ssh_packet_set_connection() where,
when it is passed a NULL pointer, it will implicitely allocate a struct
ssh and return it after then set the fds in it.
sshd.c appears to be the only place where the API is still used, so we
can place that sequence there. sshd.c calls the old function only once
during initialization (in main()) so it does appear to arrive there
while active_state is still NULL.
So when called with active_state==NULL, essentially the old function is
roughly equivalent to:
struct *ssh = ssh_alloc_session_state();
ssh_packet_set_connection(ssh, fd_in, fd_out)
active_state = ssh;
ssh.c handles this in a similar way. It calls
ssh_alloc_session_state_state directly then setting the active_state
pointer with the result (ssh.c line 2113).
When doing this in sshd.c too, which I think is pretty straightforward,
the packet_set_connection() will become obsolete.
diff --git a/opacket.c b/opacket.c
index 92e17a5..bff4c36 100755
--- a/opacket.c
+++ b/opacket.c
@@ -202,14 +202,6 @@ ssh_packet_get_cstring(struct ssh *ssh, u_int
*length_ptr)
/* Old API, that had to be reimplemented */
-void
-packet_set_connection(int fd_in, int fd_out)
-{
- active_state = ssh_packet_set_connection(active_state, fd_in, fd_out);
- if (active_state == NULL)
- fatal("%s: ssh_packet_set_connection failed", __func__);
-}
-
u_int
packet_get_char(void)
{
diff --git a/opacket.h b/opacket.h
index c6e5124..d711468 100755
--- a/opacket.h
+++ b/opacket.h
@@ -38,7 +38,6 @@ do { \
void packet_close(void);
u_int packet_get_char(void);
u_int packet_get_int(void);
-void packet_set_connection(int, int);
int packet_read_seqnr(u_int32_t *);
int packet_read_poll_seqnr(u_int32_t *);
void packet_process_incoming(const char *buf, u_int len);
diff --git a/sshd.c b/sshd.c
index 2795a2e..65b96d4 100755
--- a/sshd.c
+++ b/sshd.c
@@ -1906,9 +1906,12 @@ main(int ac, char **av)
* Register our connection. This turns encryption off because we do
* not have a key.
*/
- packet_set_connection(sock_in, sock_out);
+ if ((ssh = ssh_alloc_session_state()) == NULL)
+ fatal("Couldn't allocate session state");
+ active_state = ssh; /* XXX legacy API compat */
+ if (ssh_packet_set_connection(ssh, sock_in, sock_out) != NULL)
+ fatal("ssh_packet_set_connection failed");
packet_set_server();
- ssh = active_state; /* XXX */
check_ip_options(ssh);
--
1.9.5 (Apple Git-50.3)+GitX
Markus Schmidt
2018-Dec-04 11:54 UTC
[PATCH[ Correction -> Re: [PATCH] removing an old API.
Hello Again.
I am embarrased to say that I found that the original patch I submitted
had an error (I accidentally copied an older version of the patch to the
email).
- if (ssh_packet_set_connection(ssh, sock_in, sock_out) != NULL)
+ if (ssh_packet_set_connection(ssh, sock_in, sock_out) == NULL)
Here is the full correct patch against BSD openssh-7.9 source.
My apologies.
opacket.c | 8 --------
opacket.h | 1 -
sshd.c | 7 +++++--
3 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/opacket.c b/opacket.c
index 92e17a5..bff4c36 100755
--- a/opacket.c
+++ b/opacket.c
@@ -202,14 +202,6 @@ ssh_packet_get_cstring(struct ssh *ssh, u_int
*length_ptr)
/* Old API, that had to be reimplemented */
-void
-packet_set_connection(int fd_in, int fd_out)
-{
- active_state = ssh_packet_set_connection(active_state, fd_in, fd_out);
- if (active_state == NULL)
- fatal("%s: ssh_packet_set_connection failed", __func__);
-}
-
u_int
packet_get_char(void)
{
diff --git a/opacket.h b/opacket.h
index c6e5124..d711468 100755
--- a/opacket.h
+++ b/opacket.h
@@ -38,7 +38,6 @@ do { \
void packet_close(void);
u_int packet_get_char(void);
u_int packet_get_int(void);
-void packet_set_connection(int, int);
int packet_read_seqnr(u_int32_t *);
int packet_read_poll_seqnr(u_int32_t *);
void packet_process_incoming(const char *buf, u_int len);
diff --git a/sshd.c b/sshd.c
index 2795a2e..f063771 100755
--- a/sshd.c
+++ b/sshd.c
@@ -1906,9 +1906,12 @@ main(int ac, char **av)
* Register our connection. This turns encryption off because we do
* not have a key.
*/
- packet_set_connection(sock_in, sock_out);
+ if ((ssh = ssh_alloc_session_state()) == NULL)
+ fatal("Couldn't allocate session state");
+ active_state = ssh; /* XXX */
+ if (ssh_packet_set_connection(ssh, sock_in, sock_out) == NULL)
+ fatal("ssh_packet_set_connection failed");
packet_set_server();
- ssh = active_state; /* XXX */
check_ip_options(ssh);
On 12.03.18 19:29 , Markus Schmidt wrote:
>
>
> diff --git a/opacket.c b/opacket.c
> index 92e17a5..bff4c36 100755
> --- a/opacket.c
> +++ b/opacket.c
> @@ -202,14 +202,6 @@ ssh_packet_get_cstring(struct ssh *ssh, u_int
> *length_ptr)
>
> /* Old API, that had to be reimplemented */
>
> -void
> -packet_set_connection(int fd_in, int fd_out)
> -{
> - active_state = ssh_packet_set_connection(active_state, fd_in,
fd_out);
> - if (active_state == NULL)
> - fatal("%s: ssh_packet_set_connection failed",
__func__);
> -}
> -
> u_int
> packet_get_char(void)
> {
> diff --git a/opacket.h b/opacket.h
> index c6e5124..d711468 100755
> --- a/opacket.h
> +++ b/opacket.h
> @@ -38,7 +38,6 @@ do { \
> void packet_close(void);
> u_int packet_get_char(void);
> u_int packet_get_int(void);
> -void packet_set_connection(int, int);
> int packet_read_seqnr(u_int32_t *);
> int packet_read_poll_seqnr(u_int32_t *);
> void packet_process_incoming(const char *buf, u_int len);
> diff --git a/sshd.c b/sshd.c
> index 2795a2e..65b96d4 100755
> --- a/sshd.c
> +++ b/sshd.c
> @@ -1906,9 +1906,12 @@ main(int ac, char **av)
> * Register our connection. This turns encryption off because
we do
> * not have a key.
> */
> - packet_set_connection(sock_in, sock_out);
> + if ((ssh = ssh_alloc_session_state()) == NULL)
> + fatal("Couldn't allocate session state");
> + active_state = ssh; /* XXX legacy API compat */
> + if (ssh_packet_set_connection(ssh, sock_in, sock_out) != NULL)
> + fatal("ssh_packet_set_connection failed");
> packet_set_server();
> - ssh = active_state; /* XXX */
>
> check_ip_options(ssh);
>
>
>
>
>
>
>
On Mon, 3 Dec 2018, Markus Schmidt wrote:> > While looking for leaks I came across two old packet APIs which are easy to > remove. I'm sending patches for each separately.Please don't worry about this - we have some patches to fully remove the opacket.h API that we home to move forward with soon. Any piecemeal work on that will just collide. Thanks for looking though :) -d
On 07.12.2018 04:36, Damien Miller wrote:> On Mon, 3 Dec 2018, Markus Schmidt wrote: > > Please don't worry about this - we have some patches to fully remove the > opacket.h API that we home to move forward with soon. Any piecemeal work > on that will just collide. > > Thanks for looking though :) >Ok, got it.? Good to know too, because otherwise I would have invested some efforts in this area. Markus