NetBSD Problem Report #56569

From www@netbsd.org  Wed Dec 22 21:20:32 2021
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 0A70C1A9239
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 22 Dec 2021 21:20:32 +0000 (UTC)
Message-Id: <20211222212030.B13061A923C@mollari.NetBSD.org>
Date: Wed, 22 Dec 2021 21:20:30 +0000 (UTC)
From: smallm@sdf.org
Reply-To: smallm@sdf.org
To: gnats-bugs@NetBSD.org
Subject: ssh: local variable, limit, unused in channel_pre_open in channels.c
X-Send-Pr-Version: www-1.0

>Number:         56569
>Category:       bin
>Synopsis:       ssh: local variable, limit, unused in channel_pre_open in channels.c
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Dec 22 21:25:00 +0000 2021
>Last-Modified:  Fri Dec 24 21:30:02 +0000 2021
>Originator:     Mike Small
>Release:        9.2_STABLE
>Organization:
>Environment:
NetBSD misty.Mauritania 9.2_STABLE NetBSD 9.2_STABLE (GENERIC) #0: Sun Oct 17 20:58:39 EDT 2021  smallm@misty.Mauritania:/usr/obj/sys/arch/amd64/compile/GENERIC amd64
>Description:
In NetBSD openssh, in part of the HPN patch to channels.c, the limit local variable stopped being used for anything after the merge at revision 1.19:

http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/channels.c.diff?r1=1.18&r2=1.19&only_with_tag=MAIN

As a result the conditions for setting the read file descriptor set changed from being dependent on the minimum of ssh_packet_get_maxsize(ssh) and 2 * c->tcpwinsz to depending on c->remote_window. 

I don't know if there's a performance issue this would cause. I only noticed it while reading your source code. Recent HPN dynamic window patches no longer have the extra block at the beginning of this function. See commit 03da38ccecf14622ef in Chris Rapier's repo here: http://github.com/rapier1/openssh-portable

>How-To-Repeat:
N/A
>Fix:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56569 CVS commit: src/crypto/external/bsd/openssh/dist
Date: Fri, 24 Dec 2021 13:16:11 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Fri Dec 24 18:16:11 UTC 2021

 Modified Files:
 	src/crypto/external/bsd/openssh/dist: channels.c

 Log Message:
 PR/56569: Mike Small: Remove unused code.


 To generate a diff of this commit:
 cvs rdiff -u -r1.32 -r1.33 src/crypto/external/bsd/openssh/dist/channels.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Mike Small <smallm@sdf.org>
To: gnats-bugs@netbsd.org
Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/56569 CVS commit: src/crypto/external/bsd/openssh/dist
Date: Fri, 24 Dec 2021 21:24:36 +0000

 "Christos Zoulas" <christos@netbsd.org> writes:

 > The following reply was made to PR bin/56569; it has been noted by GNATS.
 >
 > From: "Christos Zoulas" <christos@netbsd.org>
 > To: gnats-bugs@gnats.NetBSD.org
 > Cc: 
 > Subject: PR/56569 CVS commit: src/crypto/external/bsd/openssh/dist
 > Date: Fri, 24 Dec 2021 13:16:11 -0500
 >
 >  Module Name:	src
 >  Committed By:	christos
 >  Date:		Fri Dec 24 18:16:11 UTC 2021
 >  
 >  Modified Files:
 >  	src/crypto/external/bsd/openssh/dist: channels.c
 >  
 >  Log Message:
 >  PR/56569: Mike Small: Remove unused code.
 >  
 >  
 >  To generate a diff of this commit:
 >  cvs rdiff -u -r1.32 -r1.33 src/crypto/external/bsd/openssh/dist/channels.c
 >  
 >  Please note that diffs are not public domain; they are subject to the
 >  copyright notices on the relevant files.
 >  

 Sorry if I've missed something in the tip version or otherwise, but I'm
 concerned that removing all of this will introduce a regression. It was
 mostly dead code but not entirely in that it gave c->tcpwinsz an initial
 value.

 In recent HPN patches Chris Rapier changes that struct member to a local
 variable which he populates before use in channel_check_window():
 https://github.com/rapier1/openssh-portable/blob/master/channels.c

 But that seems not the way in NetBSD code. I'm thinking the following
 condition will now always be false since c->tcpwinsz will always be zero:

 static int
 channel_check_window(struct ssh *ssh, Channel *c)
 {
 ...
 		/* adjust max window size if we are in a dynamic environment */
 		if (c->dynamic_window && (c->tcpwinsz > c->local_window_max)) {
 			/* grow the window somewhat aggressively to maintain 
 			 * pressure */
 			addition = 1.5*(c->tcpwinsz - c->local_window_max);
 			c->local_window_max += addition;
 		}
 ...
 		c->local_window += c->local_consumed + addition;
 ...
 }

 - Mike S.

Home	PR Database Search