Mailing List Archive

On Thu, 1 Jun 2023, mike tancsa wrote:

> Not sure if this is the right approach / patch, but I am able to compile

No, I’d rather convert the known value to the type-of-unknown-signedness
(might even be float).

-                           now >= c->lastused + c->inactive_deadline) {
+                           now >= c->lastused + (time_t)c->inactive_deadline) {

In general, #ifdef i386 is wrong.

bye,
//mirabilos
--
Infrastrukturexperte • tarent solutions GmbH
Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/
Telephon +49 228 54881-393 • Fax: +49 228 54881-235
HRB AG Bonn 5168 • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

****************************************************
/?\ The UTF-8 Ribbon
? ? Campaign against Mit dem tarent-Newsletter nichts mehr verpassen:
 ?  HTML eMail! Also, https://www.tarent.de/newsletter
? ? header encryption!
****************************************************
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

On 6/1/2023 11:47 AM, Thorsten Glaser wrote:
> On Thu, 1 Jun 2023, mike tancsa wrote:
>
>> Not sure if this is the right approach / patch, but I am able to compile
> No, I’d rather convert the known value to the type-of-unknown-signedness
> (might even be float).
>
> -                           now >= c->lastused + c->inactive_deadline) {
> +                           now >= c->lastused + (time_t)c->inactive_deadline) {
>
> In general, #ifdef i386 is wrong.

Thanks,

compile/build and run tested on FreeBSD 13 both i386 and AMD64 with the
following patch

--- channels.c.orig     2023-06-01 15:52:19 UTC
+++ channels.c
@@ -2566,7 +2566,7 @@ channel_handler(struct ssh *ssh, int table, struct tim
                        if (table == CHAN_PRE &&
                            c->type == SSH_CHANNEL_OPEN &&
                            c->inactive_deadline != 0 && c->lastused !=
0 &&
-                           now >= c->lastused + c->inactive_deadline) {
+                           now >= c->lastused + (time_t)
c->inactive_deadline) {
                                /* channel closed for inactivity */
                                verbose("channel %d: closing after %u
seconds "
                                    "of inactivity", c->self,

    ---Mike


_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

My inclination would be to just make the timeout signed to match
time_t. No need for ugly casts then and parse_timeout() parses the
number as an int anyway (rejecting negative values). As far as I
can see there is no way for these values to be larger than an int.

- todd

Index: channels.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/channels.c,v
retrieving revision 1.430
diff -u -p -u -r1.430 channels.c
--- channels.c 10 Mar 2023 03:01:51 -0000 1.430
+++ channels.c 1 Jun 2023 16:32:38 -0000
@@ -146,7 +146,7 @@ struct permission_set {
/* Used to record timeouts per channel type */
struct ssh_channel_timeout {
char *type_pattern;
- u_int timeout_secs;
+ int timeout_secs;
};

/* Master structure for channels state */
@@ -304,11 +304,11 @@ channel_lookup(struct ssh *ssh, int id)
*/
void
channel_add_timeout(struct ssh *ssh, const char *type_pattern,
- u_int timeout_secs)
+ int timeout_secs)
{
struct ssh_channels *sc = ssh->chanctxt;

- debug2_f("channel type \"%s\" timeout %u seconds",
+ debug2_f("channel type \"%s\" timeout %d seconds",
type_pattern, timeout_secs);
sc->timeouts = xrecallocarray(sc->timeouts, sc->ntimeouts,
sc->ntimeouts + 1, sizeof(*sc->timeouts));
@@ -332,7 +332,7 @@ channel_clear_timeouts(struct ssh *ssh)
sc->ntimeouts = 0;
}

-static u_int
+static int
lookup_timeout(struct ssh *ssh, const char *type)
{
struct ssh_channels *sc = ssh->chanctxt;
Index: channels.h
===================================================================
RCS file: /cvs/src/usr.bin/ssh/channels.h,v
retrieving revision 1.149
diff -u -p -u -r1.149 channels.h
--- channels.h 4 Mar 2023 03:22:59 -0000 1.149
+++ channels.h 1 Jun 2023 16:28:59 -0000
@@ -207,7 +207,7 @@ struct Channel {
/* Last traffic seen for OPEN channels */
time_t lastused;
/* Inactivity timeout deadline in seconds (0 = no timeout) */
- u_int inactive_deadline;
+ int inactive_deadline;
};

#define CHAN_EXTENDED_IGNORE 0
@@ -305,7 +305,7 @@ int channel_close_fd(struct ssh *, Chan
void channel_send_window_changes(struct ssh *);

/* channel inactivity timeouts */
-void channel_add_timeout(struct ssh *, const char *, u_int);
+void channel_add_timeout(struct ssh *, const char *, int);
void channel_clear_timeouts(struct ssh *);

/* mux proxy support */
Index: servconf.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/servconf.c,v
retrieving revision 1.393
diff -u -p -u -r1.393 servconf.c
--- servconf.c 24 May 2023 23:01:06 -0000 1.393
+++ servconf.c 1 Jun 2023 16:31:34 -0000
@@ -908,7 +908,7 @@ process_permitopen(struct ssh *ssh, Serv

/* Parse a ChannelTimeout clause "pattern=interval" */
static int
-parse_timeout(const char *s, char **typep, u_int *secsp)
+parse_timeout(const char *s, char **typep, int *secsp)
{
char *cp, *sdup;
int secs;
@@ -934,7 +934,7 @@ parse_timeout(const char *s, char **type
if (typep != NULL)
*typep = xstrdup(sdup);
if (secsp != NULL)
- *secsp = (u_int)secs;
+ *secsp = secs;
free(sdup);
return 0;
}
@@ -942,7 +942,8 @@ parse_timeout(const char *s, char **type
void
process_channel_timeouts(struct ssh *ssh, ServerOptions *options)
{
- u_int i, secs;
+ int secs;
+ u_int i;
char *type;

debug3_f("setting %u timeouts", options->num_channel_timeouts);
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

On Thu, 1 Jun 2023, Todd C. Miller wrote:

>My inclination would be to just make the timeout signed to match
>time_t. No need for ugly casts then and parse_timeout() parses the

For OpenSSH itself, sure. For portable OpenSSH that’s not an option;
time_t can be unsigned, signed, or even float.

Another option would be to make the variable also time_t, but that
would waste much space on contemporary platforms due to Y2038, so
I think that, here, the cast is better.

bye,
//mirabilos
--
Infrastrukturexperte • tarent solutions GmbH
Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/
Telephon +49 228 54881-393 • Fax: +49 228 54881-235
HRB AG Bonn 5168 • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

****************************************************
/?\ The UTF-8 Ribbon
? ? Campaign against Mit dem tarent-Newsletter nichts mehr verpassen:
 ?  HTML eMail! Also, https://www.tarent.de/newsletter
? ? header encryption!
****************************************************
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

On Thu, 01 Jun 2023 18:50:42 +0200, Thorsten Glaser wrote:

> For OpenSSH itself, sure. For portable OpenSSH that’s not an option;
> time_t can be unsigned, signed, or even float.
>
> Another option would be to make the variable also time_t, but that
> would waste much space on contemporary platforms due to Y2038, so
> I think that, here, the cast is better.

POSIX stopped allowing time_t to be floating point some time ago
(and I don't think anyone ever implemented it that way) but that
is completely irrelevant to the diff at hand.

Even if time_t was unsigned, this would not be a problem for the
comparison in question. The result of "c->lastused + c->inactive_deadline"
would be unsigned if time_t is unsigned so there is no need to cast
c->inactive_deadline that I can see.

- todd
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev