Index | Thread | Search

From:
Damien Miller <djm@mindrot.org>
Subject:
Re: openssh: fractional-second PerSourcePenalties part 1
To:
tech@openbsd.org
Cc:
openssh@openssh.com
Date:
Mon, 1 Dec 2025 15:12:32 +1100

Download raw body.

Thread
On Fri, 28 Nov 2025, Damien Miller wrote:

> 
> Hi,
> 
> I'd like to add fractional second penalties to PerSourcePenalties,
> and this is part 1:
> 
> We use mis.c:convtime() to parse intervals and durations like
> "1h30m15s" and so forth. This is used to parse PerSourcePenalties
> times.
> 
> To support fractional second penalties, this adds convtime_usec()
> that allows things like "1h30m15.123s".
> 
> It only allows fractions in the seconds qualifier, i.e. "0.5w" is
> disallowed. Obvious junk like "1.5s123" is also disallowed.
> 
> This also rewrites the existing convtime() function to use
> convtime_usec() internally since they're so similar and the
> latter is more general. I got rid of the MINUTES/HOURS/etc defines
> because they are pretty obvious.
> 
> ok?

Darren asked what this would look like with the funky fixed point
parsing ripped out and replaced with strtod(). Turns out it's much
simpler.

I had avoided this initially because I wanted to avoid the need to
use libm which seemed necessary to detect fractional seconds in
places where we don't want them, but that can be done by just
looking for '.' in the string. We need to do that anyway for some
other validation.

diff --git a/misc.c b/misc.c
index 3cc7add..18e56bc 100644
--- a/misc.c
+++ b/misc.c
@@ -576,25 +576,13 @@ a2tun(const char *s, int *remote)
 	return (tun);
 }
 
-#define SECONDS		1
-#define MINUTES		(SECONDS * 60)
-#define HOURS		(MINUTES * 60)
-#define DAYS		(HOURS * 24)
-#define WEEKS		(DAYS * 7)
-
-static char *
-scandigits(char *s)
-{
-	while (isdigit((unsigned char)*s))
-		s++;
-	return s;
-}
-
 /*
- * Convert a time string into seconds; format is
- * a sequence of:
+ * Convert a time string into microseconds; format is a sequence of:
  *      time[qualifier]
  *
+ * This supports fractional values for the seconds value only. All other
+ * values must be integers.
+ *
  * Valid time qualifiers are:
  *      <none>  seconds
  *      s|S     seconds
@@ -604,79 +592,115 @@ scandigits(char *s)
  *      w|W     weeks
  *
  * Examples:
- *      90m     90 minutes
- *      1h30m   90 minutes
- *      2d      2 days
- *      1w      1 week
+ *      90m      90 minutes
+ *      1h30m    90 minutes
+ *      1.5s     1.5 seconds
+ *      2d       2 days
+ *      1w       1 week
  *
- * Return -1 if time string is invalid.
+ * Returns -1 if the time string is invalid.
  */
-int
-convtime(const char *s)
+int64_t
+convtime_usec(const char *s)
 {
-	int secs, total = 0, multiplier;
-	char *p, *os, *np, c = 0;
-	const char *errstr;
+	int64_t val_usec, total_usec = 0;
+	const char *p, *start_p;
+	char *endp;
+	int seen_seconds = 0;
+	double val, multiplier_sec;
 
 	if (s == NULL || *s == '\0')
 		return -1;
-	p = os = strdup(s);	/* deal with const */
-	if (os == NULL)
-		return -1;
 
-	while (*p) {
-		np = scandigits(p);
-		if (np) {
-			c = *np;
-			*np = '\0';
-		}
-		secs = (int)strtonum(p, 0, INT_MAX, &errstr);
-		if (errstr)
-			goto fail;
-		*np = c;
+	for (p = s; *p != '\0';) {
+		if (!isdigit((unsigned char)*p) && *p != '.')
+			return -1;
 
-		multiplier = 1;
-		switch (c) {
+		errno = 0;
+		if ((val = strtod(p, &endp)) < 0 || errno != 0 || p == endp)
+			return -1;
+		start_p = p;
+		p = endp;
+
+		switch (*p) {
 		case '\0':
-			np--;	/* back up */
-			break;
+			/* FALLTHROUGH */
 		case 's':
 		case 'S':
+			if (seen_seconds++)
+				return -1;
+			multiplier_sec = 1;
 			break;
 		case 'm':
 		case 'M':
-			multiplier = MINUTES;
+			multiplier_sec = 60;
 			break;
 		case 'h':
 		case 'H':
-			multiplier = HOURS;
+			multiplier_sec = 60 * 60;
 			break;
 		case 'd':
 		case 'D':
-			multiplier = DAYS;
+			multiplier_sec = 24 * 60 * 60;
 			break;
 		case 'w':
 		case 'W':
-			multiplier = WEEKS;
+			multiplier_sec = 7 * 24 * 60 * 60;
 			break;
 		default:
-			goto fail;
+			return -1;
 		}
-		if (secs > INT_MAX / multiplier)
-			goto fail;
-		secs *= multiplier;
-		if  (total > INT_MAX - secs)
-			goto fail;
-		total += secs;
-		if (total < 0)
-			goto fail;
-		p = ++np;
+
+		/* Special handling of this was a decimal */
+		if (memchr(start_p, '.', endp - start_p) != NULL) {
+			/* Decimal point present */
+			if (multiplier_sec > 1)
+				return -1; /* No fractionals for non-seconds */
+			/* For seconds, ensure digits follow */
+			if (!isdigit((unsigned char)*(endp - 1)))
+				return -1;
+		}
+
+		double temp_val_usec_d = val * multiplier_sec * 1000000.0;
+		if (temp_val_usec_d < 0 || temp_val_usec_d > (double)INT64_MAX)
+			return -1;
+
+		val_usec = (int64_t)temp_val_usec_d;
+
+		if (total_usec > INT64_MAX - val_usec)
+			return -1; /* overflow */
+		total_usec += val_usec;
+
+		if (*p != '\0')
+			p++;
 	}
-	free(os);
-	return total;
-fail:
-	free(os);
-	return -1;
+	return total_usec;
+}
+
+/*
+ * Same as convtime_usec() above but fractional seconds are not supported.
+ * Return -1 if time string is invalid.
+ */
+int
+convtime(const char *s)
+{
+	int64_t usec_val;
+
+	if ((usec_val = convtime_usec(s)) == -1)
+		return -1;
+
+	/* Check for fractional seconds: convtime does not support them */
+	if (usec_val % 1000000 != 0)
+		return -1;
+
+	/* Convert to seconds */
+	usec_val /= 1000000;
+
+	/* Check for overflow into int */
+	if (usec_val < 0 || usec_val > INT_MAX)
+		return -1;
+
+	return (int)usec_val;
 }
 
 #define TF_BUFS	8
diff --git a/misc.h b/misc.h
index 6dd0dc0..bb8844b 100644
--- a/misc.h
+++ b/misc.h
@@ -82,6 +82,7 @@ int	 parse_user_host_path(const char *, char **, char **, char **);
 int	 parse_user_host_port(const char *, char **, char **, int *);
 int	 parse_uri(const char *, const char *, char **, char **, int *, char **);
 int	 convtime(const char *);
+int64_t	 convtime_usec(const char *);
 const char *fmt_timeframe(time_t t);
 int	 tilde_expand(const char *, uid_t, char **);
 char	*tilde_expand_filename(const char *, uid_t);